On Wed, Sep 3, 2008 at 2:28 AM, Ohad Lutzky <[EMAIL PROTECTED]> wrote:
> From flood_report_relative_times.c (SVN):
>
> /* FIXME: this call may need to be in a critical section */
> #if APR_HAS_THREADS
> apr_file_printf(local_stdout, "%s %ld %s\n", buf,
> apr_os_thread_current(), req->uri);
> #else
> apr_file_printf(local_stdout, "%s %d %s\n", buf, getpid(), req->uri);
> #endif
>
> The comment is right - it does need to be in a critical section. For
> the case of multiple processes, flock can be used. However, for the
> case of a single process, this doesn't work, and a mutex would be in
> order. What would be the correct way to go about organizing such a
> per-process mutex? Note that apr_file_printf indirectly calls
> apr_file_write, which is thread-safe - however, it call it multiple
> times, which can (and does) lead to interlacing in output.
Yup, I've seen that before, but I generally tend to ignore it. =)
I believe the best way would be to add a mutex in the config_t
structure and lock that in the report function. Rough patch below.
What do you think? -- justin
Index: flood_farm.c
===================================================================
--- flood_farm.c (revision 695997)
+++ flood_farm.c (working copy)
@@ -231,6 +231,7 @@ apr_status_t run_farm(config_t *config, const char
#if APR_HAS_THREADS
farm->farmers = apr_pcalloc(pool,
sizeof(apr_thread_t*) * (usefarmer_count + 1));
+ apr_thread_mutex_create(&config->mutex, APR_THREAD_MUTEX_DEFAULT, pool);
#else
farm->farmers = apr_pcalloc(pool,
sizeof(apr_proc_t*) * (usefarmer_count + 1));
Index: flood_config.h
===================================================================
--- flood_config.h (revision 695997)
+++ flood_config.h (working copy)
@@ -31,6 +31,9 @@
typedef struct {
apr_xml_doc *doc;
apr_pool_t *pool;
+#if APR_HAS_THREADS
+ apr_thread_mutex_t *mutex;
+#endif
} config_t;
/**
Index: flood_report_relative_times.c
===================================================================
--- flood_report_relative_times.c (revision 695997)
+++ flood_report_relative_times.c (working copy)
@@ -28,9 +28,18 @@
extern apr_file_t *local_stdout;
extern apr_file_t *local_stderr;
+struct relative_report_t {
+ config_t *config;
+};
+typedef struct relative_report_t relative_report_t;
+
apr_status_t relative_times_report_init(report_t **report, config_t *config,
const char *profile_name, apr_pool_t *pool)
{
+ relative_report_t *rr = apr_palloc(pool, sizeof(relative_report_t));
+ rr->config = config;
+
+ *report = rr;
return APR_SUCCESS;
}
@@ -39,6 +48,7 @@ apr_status_t relative_times_process_stats(report_t
#define FLOOD_PRINT_BUF 256
apr_size_t buflen;
char buf[FLOOD_PRINT_BUF];
+ relative_report_t *rr = (relative_report_t*)report;
buflen = apr_snprintf(buf, FLOOD_PRINT_BUF,
"%" APR_INT64_T_FMT " %" APR_INT64_T_FMT
@@ -61,9 +71,10 @@ apr_status_t relative_times_process_stats(report_t
apr_snprintf(buf+buflen, FLOOD_PRINT_BUF-buflen, " %d ", verified);
}
- /* FIXME: this call may need to be in a critical section */
#if APR_HAS_THREADS
+ apr_thread_mutex_lock(rr->config->mutex);
apr_file_printf(local_stdout, "%s %ld %s\n", buf,
apr_os_thread_current(), req->uri);
+ apr_thread_mutex_unlock(rr->config->mutex);
#else
apr_file_printf(local_stdout, "%s %d %s\n", buf, getpid(), req->uri);
#endif
