httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: svn commit: r1068581 - in /httpd/httpd/trunk/modules/proxy: mod_proxy.h mod_proxy_balancer.c mod_proxy_ftp.c proxy_util.c
Date Wed, 09 Feb 2011 07:33:29 GMT


On 02/08/2011 10:08 PM, jim@apache.org wrote:
> Author: jim
> Date: Tue Feb  8 21:08:10 2011
> New Revision: 1068581
> 
> URL: http://svn.apache.org/viewvc?rev=1068581&view=rev
> Log:
> Remove the thread mutex from the worker... it really should be
> in the balancer. Thus we have global and thread for the balancer.
> Use global when updating the full, shm list of workers; use
> thread when being local.
> 
> Modified:
>     httpd/httpd/trunk/modules/proxy/mod_proxy.h
>     httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
>     httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c
>     httpd/httpd/trunk/modules/proxy/proxy_util.c
> 
> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy.h
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy.h?rev=1068581&r1=1068580&r2=1068581&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/mod_proxy.h (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy.h Tue Feb  8 21:08:10 2011
> @@ -375,7 +376,6 @@ struct proxy_worker {
>      proxy_conn_pool     *cp;    /* Connection pool to use */
>      proxy_worker_shared   *s;   /* Shared data */
>      proxy_balancer  *balancer;  /* which balancer am I in? */
> -    apr_thread_mutex_t  *mutex; /* Thread lock for updating address cache */


Don't we need to protect the address cache for unbalanced workers as well by a thread lock?


>      void            *context;   /* general purpose storage */
>  };
>  
> 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=1068581&r1=1068580&r2=1068581&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c Tue Feb  8 21:08:10 2011
>> @@ -336,12 +336,7 @@ static proxy_worker *find_best_worker(pr
>      if (candidate)
>          candidate->s->elected++;

Hm, why don't we need to protect the data in the shared mem like candidate->s->elected
or the other elements that are touched by the finder with a global lock?


>  
> -/*
> -        PROXY_GLOBAL_UNLOCK(conf);
> -        return NULL;
> -*/
> -
> -    if ((rv = PROXY_GLOBAL_UNLOCK(balancer)) != APR_SUCCESS) {
> +    if ((rv = PROXY_THREAD_UNLOCK(balancer)) != APR_SUCCESS) {
>          ap_log_error(APLOG_MARK, APLOG_ERR, rv, r->server,
>          "proxy: BALANCER: (%s). Unlock failed for find_best_worker()", balancer->name);
>      }
> @@ -463,7 +458,7 @@ static int proxy_balancer_pre_request(pr
>      /* Step 2: Lock the LoadBalancer
>       * XXX: perhaps we need the process lock here
>       */
> -    if ((rv = PROXY_GLOBAL_LOCK(*balancer)) != APR_SUCCESS) {
> +    if ((rv = PROXY_THREAD_LOCK(*balancer)) != APR_SUCCESS) {

Same question as above: We change data in the shared mem. So why only a thread lock?

>          ap_log_error(APLOG_MARK, APLOG_ERR, rv, r->server,
>                       "proxy: BALANCER: (%s). Lock failed for pre_request",
>                       (*balancer)->name);
> @@ -614,7 +609,7 @@ static int proxy_balancer_post_request(p
>  
>      apr_status_t rv;
>  
> -    if ((rv = PROXY_GLOBAL_LOCK(balancer)) != APR_SUCCESS) {
> +    if ((rv = PROXY_THREAD_UNLOCK(balancer)) != APR_SUCCESS) {

I assume you wanted PROXY_THREAD_LOCK instead of PROXY_THREAD_UNLOCK :-)

Same question as above: We change data in the shared mem. So why only a thread lock?


>          ap_log_error(APLOG_MARK, APLOG_ERR, rv, r->server,
>              "proxy: BALANCER: (%s). Lock failed for post_request",
>              balancer->name);
> @@ -878,13 +875,13 @@ static int balancer_handler(request_rec 
>      balancer = (proxy_balancer *)conf->balancers->elts;
>      for (i = 0; i < conf->balancers->nelts; i++, balancer++) {
>          apr_status_t rv;
> -        if ((rv = PROXY_GLOBAL_LOCK(balancer)) != APR_SUCCESS) {
> +        if ((rv = PROXY_THREAD_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);

Can't the data change in shared memory while we update our local data, e.g. by balancer_handler
that
is executed in another process and that adds or removes members at the same time?

> -        if ((rv = PROXY_GLOBAL_UNLOCK(balancer)) != APR_SUCCESS) {
> +        if ((rv = PROXY_THREAD_UNLOCK(balancer)) != APR_SUCCESS) {
>              ap_log_error(APLOG_MARK, APLOG_ERR, rv, r->server,
>                           "proxy: BALANCER: (%s). Unlock failed for balancer_handler",
>                           balancer->name);


Regards

RĂ¼diger

Mime
View raw message