On Feb 7, 2011, at 9:54 PM, Roy T. Fielding wrote: > On Feb 4, 2011, at 12:34 PM, [email protected] 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 !
