httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Roy T. Fielding" <roy.field...@gmail.com>
Subject Re: svn commit: r1067276 - /httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
Date Tue, 08 Feb 2011 02:54:53 GMT
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.

....Roy




Mime
View raw message