You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by ji...@apache.org on 2011/02/04 21:34:48 UTC
svn commit: r1067276 - /httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
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);
+ }
+ }
if (r->args) {
char *args = apr_pstrdup(r->pool, r->args);
Re: svn commit: r1067276 - /httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
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 !
Re: svn commit: r1067276 - /httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
Posted by "Roy T. Fielding" <ro...@gmail.com>.
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