httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jim Jagielski <...@jaguNET.com>
Subject Re: svn commit: r1067276 - /httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
Date Tue, 08 Feb 2011 13:01:08 GMT

On Feb 7, 2011, at 9:54 PM, Roy T. Fielding wrote:

> On Feb 4, 2011, at 12:34 PM, jim@apache.org wrote:
> 
>> Author: jim
>> Date: Fri Feb  4 20:34:47 2011
>> New Revision: 1067276
>> 
>> URL: http://svn.apache.org/viewvc?rev=1067276&view=rev
>> Log:
>> Lock around the time when we're mucking w/ balancers...
>> 
>> Modified:
>>   httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
>> 
>> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c?rev=1067276&r1=1067275&r2=1067276&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c (original)
>> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c Fri Feb  4 20:34:47 2011
>> @@ -876,8 +876,20 @@ static int balancer_handler(request_rec 
>>    params = apr_table_make(r->pool, 10);
>> 
>>    balancer = (proxy_balancer *)conf->balancers->elts;
>> -    for (i = 0; i < conf->balancers->nelts; i++, balancer++)
>> +    for (i = 0; i < conf->balancers->nelts; i++, balancer++) {
>> +        apr_status_t rv;
>> +        if ((rv = PROXY_GLOBAL_LOCK(balancer)) != APR_SUCCESS) {
>> +            ap_log_error(APLOG_MARK, APLOG_ERR, rv, r->server,
>> +                         "proxy: BALANCER: (%s). Lock failed for balancer_handler",
>> +                         balancer->name);
>> +        }
>>        ap_proxy_update_members(balancer, r->server, conf);
>> +        if ((rv = PROXY_GLOBAL_UNLOCK(balancer)) != APR_SUCCESS) {
>> +            ap_log_error(APLOG_MARK, APLOG_ERR, rv, r->server,
>> +                         "proxy: BALANCER: (%s). Unlock failed for balancer_handler",
>> +                         balancer->name);
>> +        }
>> +    }
> 
> Umm, yuck.  That doesn't seem right.
> 
> If the lock is needed, then an update should not be done when
> the lock fails.  And what conditions would cause us to fail an unlock?
> It sounds fatal.
> 
> I don't see why a lock is needed here, but I haven't looked at the context
> enough to understand what ap_proxy_update_members() is doing (or if it
> could be designed lock-free).  I just hate to see a loop of locking, even
> though I am clueless about the balancer.

Well, there are a coupla/few other places in httpd where we
either simply ignore the return status of lock/unlock or
simply, as above, log the error and hope for the best.

To be honest, I am smoke-testing to see if the lock is even
required... It's only really when we "grab" a shm segment
that is critical to avoid overlaps.

Thx for the comments and the extra eyes !
Mime
View raw message