Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (athena.apache.org: domain of ylavic.dev@gmail.com
 designates 209.85.213.181 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <54F9C6EF.9000702@apache.org>
References: <54F880F2.9060504@apache.org>
	<CALK=YjPWJAD8SNxrFTEuY75WDHwkWOzcJaU2S20_RLwwaqeM2w@mail.gmail.com>
	<54F88B92.1010708@apache.org>
	<54F9C6EF.9000702@apache.org>
Date: Fri, 6 Mar 2015 17:01:35 +0100
Message-ID: 
 <CAKQ1sVMU_2HftyvVNSQgwwWwPAicuBqp5222V5LVP07CsJyoNA@mail.gmail.com>
Subject: Re: error_time reset in proxy_util.c
From: Yann Ylavic <ylavic.dev@gmail.com>
To: httpd <dev@httpd.apache.org>
Content-Type: text/plain; charset=UTF-8

On Fri, Mar 6, 2015 at 4:25 PM, Ruediger Pluem <rpluem@apache.org> wrote:
>
> Index: proxy_util.c
> ===================================================================
> --- proxy_util.c        (revision 1664261)
> +++ proxy_util.c        (working copy)
> @@ -2887,14 +2887,15 @@
>
>          connected    = 1;
>      }
> +    if (PROXY_WORKER_IS_USABLE(worker)) {
>      /*
>       * Put the entire worker to error state if
>       * the PROXY_WORKER_IGNORE_ERRORS flag is not set.
>       * Altrough some connections may be alive
>       * no further connections to the worker could be made
>       */
> -    if (!connected && PROXY_WORKER_IS_USABLE(worker) &&
> -        !(worker->s->status & PROXY_WORKER_IGNORE_ERRORS)) {
> +        if (!connected) {
> +            if (!(worker->s->status & PROXY_WORKER_IGNORE_ERRORS)) {
>          worker->s->error_time = apr_time_now();
>          worker->s->status |= PROXY_WORKER_IN_ERROR;
>          ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(00959)
> @@ -2902,6 +2903,7 @@
>              APR_TIME_T_FMT "s",
>              worker->s->hostname, apr_time_sec(worker->s->retry));
>      }
> +        }
>      else {
>          if (worker->s->retries) {
>              /*
> @@ -2915,6 +2917,19 @@
>      }
>      return connected ? OK : DECLINED;
>  }
> +    else {
> +        /*
> +         * The worker is in error likely done by a different thread / process
> +         * e.g. for a timeout or bad status. We should respect this and should
> +         * not continue with a connection via this worker even if we got one.
> +         */
> +        if (connected) {
> +            apr_socket_close(conn->sock );
> +            conn->sock = NULL;
> +        }
> +        return DECLINED;

If the worker is *not a BalancerMember*, couldn't we simply mark the
connection as close = 1 here, and continue with it until the end of
this request?
This is to avoid responding 503 whereas it succeed for this particular
request (since this is a race, the worker was put in error state a
very little time ago, not at ap_proxy_acquire_connection() time for
this request at least).

Otherwise, I agree with the move of :
+            worker->s->error_time = 0;
+            worker->s->retries = 0;
in the correct block.

Regards,
Yann.