httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Justin Erenkrantz" <jus...@erenkrantz.com>
Subject Re: [flood] Critical section for flood_report_relative_times.c
Date Tue, 16 Sep 2008 18:45:12 GMT
On Wed, Sep 3, 2008 at 2:28 AM, Ohad Lutzky <ohad@lutzky.net> 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

Mime
View raw message