Commit 243d5eab authored by Jim Jagielski's avatar Jim Jagielski
Browse files

Merge r1664709, r1697323 from trunk: 

 * Do not reset the retry timeout if the worker is in error at this stage even
   if the connection to the backend was successful. It was likely set into
   error by a different thread / process in parallel 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.


* Do a more complete cleanup here. At this point we cannot end up with something useful with the data we created so far.
Submitted by: rpluem
Reviewed/backported by: jim


git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1704835 13f79535-47bb-0310-9956-ffa450edef68
parent 9aea9b46

CHANGES

+3 −0
Original line number Diff line number Diff line
@@ -2,6 +2,9 @@ 


Changes with Apache 2.4.17



  *) mod_proxy: Fix a race condition that caused a failed worker to be retried

     before the retry period is over. [Ruediger Pluem]



  *) mod_autoindex: Allow autoindexes when neither mod_dir nor mod_mime are

     loaded. [Eric Covener]

STATUS

+1 −21
Original line number Diff line number Diff line
@@ -109,26 +109,6 @@ RELEASE SHOWSTOPPERS: 
PATCHES ACCEPTED TO BACKPORT FROM TRUNK:

  [ start all new proposals below, under PATCHES PROPOSED. ]

 

  *) mod_proxy: Fix a race condition that caused a failed worker to be retried

     before the retry period is over

      Trunk version of patch:

         http://svn.apache.org/r1664709

         http://svn.apache.org/r1697323

      Backport version for 2.4.x of patch:

         Trunk version of patch works modulo CHANGES

      +1: rpluem, ylavic, jim

      niq: 1. the if(worker->s->retries) {} and comment at line 2917

              don't seem to make any sense.

      rpluem: This is just taken over from existing code. It is just indented

              differently hence part of the path I think it should be marked

              as TODO section. But this should be subject to another

              patch.

           2. Re: error handline line 2930 - can PROXY_WORKER_IS_USABLE

              not be tested BEFORE opening connection?

      rpluem: We could, but we can catch more race cases with the current code

              as it also catches the case where a connection establishment

              took long and the worker went into error meanwhile.

 

 



PATCHES PROPOSED TO BACKPORT FROM TRUNK:

modules/proxy/proxy_util.c

+37 −23
Original line number Diff line number Diff line
@@ -2826,14 +2826,15 @@ PROXY_DECLARE(int) ap_proxy_connect_backend(const char *proxy_function, 


        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

         * Although 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)
@@ -2841,6 +2842,7 @@ PROXY_DECLARE(int) ap_proxy_connect_backend(const char *proxy_function, 
                    APR_TIME_T_FMT "s",

                    worker->s->hostname, apr_time_sec(worker->s->retry));

            }

        }

        else {

            if (worker->s->retries) {

                /*
@@ -2854,6 +2856,18 @@ PROXY_DECLARE(int) ap_proxy_connect_backend(const char *proxy_function, 
        }

        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) {

            socket_cleanup(conn);

        }

        return DECLINED;

    }

}



static apr_status_t connection_shutdown(void *theconn)

{