You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ohad Lutzky <oh...@lutzky.net> on 2008/09/03 11:28:35 UTC
[flood] Critical section for flood_report_relative_times.c
>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.
--
Man is the only animal that laughs and weeps, for he is the only
animal that is struck with the difference between what things are and
what they ought to be.
- William Hazlitt
Ohad Lutzky
Re: [flood] Critical section for flood_report_relative_times.c
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On Sat, Sep 20, 2008 at 11:40 PM, Ohad Lutzky <oh...@lutzky.net> wrote:
>> *SNIP* patch *SNIP*
>
> The patch seems to work well, thanks! :)
Cool - committed in r697920. Thanks. -- justin
Re: [flood] Critical section for flood_report_relative_times.c
Posted by Ohad Lutzky <oh...@lutzky.net>.
On Tue, Sep 16, 2008 at 9:45 PM, Justin Erenkrantz
<ju...@erenkrantz.com> wrote:
> On Wed, Sep 3, 2008 at 2:28 AM, Ohad Lutzky <oh...@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)
> *SNIP* patch *SNIP*
The patch seems to work well, thanks! :)
--
Man is the only animal that laughs and weeps, for he is the only
animal that is struck with the difference between what things are and
what they ought to be.
- William Hazlitt
Ohad Lutzky
Re: [flood] Critical section for flood_report_relative_times.c
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On Wed, Sep 3, 2008 at 2:28 AM, Ohad Lutzky <oh...@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