You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2009/07/30 21:27:12 UTC
Re: svn commit: r799334 - /httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c
On 07/30/2009 05:37 PM, jfclere@apache.org wrote:
> Author: jfclere
> Date: Thu Jul 30 15:37:22 2009
> New Revision: 799334
>
> URL: http://svn.apache.org/viewvc?rev=799334&view=rev
> Log:
> Add the file logic for the handler.
> (Next step add the slotmem logic for the multicast socket).
>
> Modified:
> httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c
>
> Modified: httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c?rev=799334&r1=799333&r2=799334&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c (original)
> +++ httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c Thu Jul 30 15:37:22 2009
> +static apr_status_t hm_file_update_stat(hm_ctx_t *ctx, hm_server_t *s, apr_pool_t *pool)
> +{
> + apr_status_t rv;
> + apr_file_t *fp;
> + apr_file_t *fpin;
> + apr_hash_index_t *hi;
> + apr_time_t now;
> + unsigned int fage;
> + apr_finfo_t fi;
> + int updated = 0;
> + char *path = apr_pstrcat(pool, ctx->storage_path, ".tmp.XXXXXX", NULL);
> +
> +
> + /* TODO: Update stats file (!) */
> + rv = apr_file_mktemp(&fp, path, APR_CREATE | APR_WRITE, pool);
> +
> + if (rv) {
> + ap_log_error(APLOG_MARK, APLOG_CRIT, rv, ctx->s,
> + "Heartmonitor: Unable to open tmp file: %s", path);
> + return rv;
> + }
> + rv = apr_file_open(&fpin, ctx->storage_path, APR_READ|APR_BINARY|APR_BUFFERED,
> + APR_OS_DEFAULT, pool);
> +
> + now = apr_time_now();
> + if (rv == APR_SUCCESS) {
> + char *t;
> + apr_table_t *hbt = apr_table_make(pool, 10);
> + apr_bucket_alloc_t *ba = apr_bucket_alloc_create(pool);
> + apr_bucket_brigade *bb = apr_brigade_create(pool, ba);
> + apr_bucket_brigade *tmpbb = apr_brigade_create(pool, ba);
> + rv = apr_file_info_get(&fi, APR_FINFO_SIZE | APR_FINFO_MTIME, fpin);
> + if (rv) {
> + ap_log_error(APLOG_MARK, APLOG_CRIT, rv, ctx->s,
> + "Heartmonitor: Unable to read file: %s", ctx->storage_path);
> + return rv;
> + }
> +
> + /* Read the file and update the line corresponding to the node */
> + ba = apr_bucket_alloc_create(pool);
> + bb = apr_brigade_create(pool, ba);
> + apr_brigade_insert_file(bb, fpin, 0, fi.size, pool);
> + tmpbb = apr_brigade_create(pool, ba);
> + fage = (unsigned int) apr_time_sec(now - fi.mtime);
> + do {
> + char buf[4096];
> + const char *ip;
> + apr_size_t bsize = sizeof(buf);
> + apr_brigade_cleanup(tmpbb);
> + if (APR_BRIGADE_EMPTY(bb)) {
> + break;
> + }
> + rv = apr_brigade_split_line(tmpbb, bb,
> + APR_BLOCK_READ, sizeof(buf));
> +
> + if (rv) {
> + ap_log_error(APLOG_MARK, APLOG_CRIT, rv, ctx->s,
> + "Heartmonitor: Unable to read from file: %s", ctx->storage_path);
> + return rv;
> + }
> +
> + apr_brigade_flatten(tmpbb, buf, &bsize);
> + if (bsize == 0) {
> + break;
> + }
> + buf[bsize - 1] = 0;
> + t = strchr(buf, ' ');
> + if (t) {
> + ip = apr_pstrndup(pool, buf, t - buf);
> + } else {
> + ip = NULL;
> + }
> + if (!ip || buf[0] == '#') {
> + /* copy things we can't process */
> + apr_file_printf(fp, "%s\n", buf);
> + } else if (strcmp(ip, s->ip) !=0 ) {
> + hm_server_t node;
> + unsigned int seen;
> + /* Update seen time according to the last file modification */
> + apr_table_clear(hbt);
> + argstr_to_table(pool, apr_pstrdup(pool, t), hbt);
> + if (apr_table_get(hbt, "busy")) {
> + node.busy = atoi(apr_table_get(hbt, "busy"));
> + }
> +
> + if (apr_table_get(hbt, "ready")) {
> + node.ready = atoi(apr_table_get(hbt, "ready"));
> + }
> +
> + if (apr_table_get(hbt, "lastseen")) {
> + node.seen = atoi(apr_table_get(hbt, "lastseen"));
> + }
> + seen = fage + node.seen;
> + apr_file_printf(fp, "%s &ready=%u&busy=%u&lastseen=%u\n",
> + ip, node.ready, node.busy, (unsigned int) seen);
The above seems to be a lot of parsing back and forth for just updating the lastseen parameter.
Couldn't we just keep the complete line until the 'lastseen=' and just update the number afterwards?
Or is the order of ready, busy and lastseen not defined?
> + }
> +
> + rv = apr_file_rename(path, ctx->storage_path, pool);
We need to consider that multiple handlers run in parallel in different threads / processes.
If we are in bad luck this can lock out node A as in the following example.
Node A and Node B sent the request at the same time and the handlers processing these requests are
running fairly in parallel.
If the handler processing Node B does the renaming one tick later then the handler processing
Node A then the update of Node A is lost. If this happens over and over again Node A is believed
to be dead although it is not.
I fear you need to use global locks here to avoid this.
Regards
Rüdiger
Re: svn commit: r799334 - /httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 07/30/2009 09:27 PM, Ruediger Pluem wrote:
>
> On 07/30/2009 05:37 PM, jfclere@apache.org wrote:
>> Author: jfclere
>> Date: Thu Jul 30 15:37:22 2009
>> New Revision: 799334
>>
>> URL: http://svn.apache.org/viewvc?rev=799334&view=rev
>> Log:
>> Add the file logic for the handler.
>> (Next step add the slotmem logic for the multicast socket).
>>
>> Modified:
>> httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c
Plus we get the following warnings:
mod_heartmonitor.c: In function 'hm_file_update_stat':
mod_heartmonitor.c:239: warning: unused variable 'hi'
mod_heartmonitor.c: In function 'hm_file_update_stats':
mod_heartmonitor.c:411: warning: dereferencing type-punned pointer will break st
rict-aliasing rules
mod_heartmonitor.c: In function 'hm_handler':
mod_heartmonitor.c:310: warning: 'node.busy' may be used uninitialized in this f
unction
mod_heartmonitor.c:310: warning: 'node.ready' may be used uninitialized in this
function
mod_heartmonitor.c:310: warning: 'node.seen' may be used uninitialized in this f
unction
Regards
Rüdiger
Re: svn commit: r799334 - /httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c
Posted by jean-frederic clere <jf...@gmail.com>.
On 07/30/2009 09:27 PM, Ruediger Pluem wrote:
>
> On 07/30/2009 05:37 PM, jfclere@apache.org wrote:
>> Author: jfclere
>> Date: Thu Jul 30 15:37:22 2009
>> New Revision: 799334
>>
>> URL: http://svn.apache.org/viewvc?rev=799334&view=rev
>> Log:
>> Add the file logic for the handler.
>> (Next step add the slotmem logic for the multicast socket).
>>
>> Modified:
>> httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c
>>
>> Modified: httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c?rev=799334&r1=799333&r2=799334&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c (original)
>> +++ httpd/httpd/trunk/modules/cluster/mod_heartmonitor.c Thu Jul 30 15:37:22 2009
>
+++ CUT +++
>> +
>> + if (apr_table_get(hbt, "lastseen")) {
>> + node.seen = atoi(apr_table_get(hbt, "lastseen"));
>> + }
>> + seen = fage + node.seen;
>> + apr_file_printf(fp, "%s&ready=%u&busy=%u&lastseen=%u\n",
>> + ip, node.ready, node.busy, (unsigned int) seen);
>
> The above seems to be a lot of parsing back and forth for just updating the lastseen parameter.
> Couldn't we just keep the complete line until the 'lastseen=' and just update the number afterwards?
If only mod_heartmonitor writes there yes otherwise no.
Probably looking for lastseen= and digits should be enough.
> Or is the order of ready, busy and lastseen not defined?
If the order is defined we could use a structure but we would loose all
the flexibility the actual format allows.
>
>
>> + }
>> +
>> + rv = apr_file_rename(path, ctx->storage_path, pool);
>
> We need to consider that multiple handlers run in parallel in different threads / processes.
> If we are in bad luck this can lock out node A as in the following example.
>
> Node A and Node B sent the request at the same time and the handlers processing these requests are
> running fairly in parallel.
> If the handler processing Node B does the renaming one tick later then the handler processing
> Node A then the update of Node A is lost. If this happens over and over again Node A is believed
> to be dead although it is not.
> I fear you need to use global locks here to avoid this.
Yes, I will work of that asap.
Cheers
Jean-Frederic