mod_proxy: follow up to r1750392.

Avoid double checking the connection in ap_proxy_connect_backend() when
ap_proxy_check_backend() says it is up and good to go.

This can be done by moving the PROXY_WORKER_IS_USABLE() check in
ap_proxy_check_backend(), since it is called by ap_proxy_connect_backend(),
and not calling the latter if the former succeeded (for the modules using it).

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1750474 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/modules/proxy/mod_proxy.h b/modules/proxy/mod_proxy.h
index f606b18ed4c2afbcd7b456f303bd1d8a9dd5e68f..8184d078353bbdb6fc324e1f3176efcd249d2d5f 100644 (file)
--- a/modules/proxy/mod_proxy.h
+++ b/modules/proxy/mod_proxy.h
@@ -980,8 +980,11 @@ PROXY_DECLARE(int) ap_proxy_release_connection(const char *proxy_function,
  * @param conn    acquired connection
  * @param s       current server record
  * @param expect_empty whether to check for empty (no data available) or not
- * @return        APR_SUCCESS or error status (APR_ENOTEMPTY if expect_empty
- *                is set but the connection is not empty)
+ * @return        APR_SUCCESS or,
+ *                APR_ENOTSOCK: not connected,
+ *                APR_NOTFOUND: worker in error state (unusable),
+ *                APR_ENOTEMPTY: expect_empty set but the connection has data,
+ *                other: connection closed/aborted (remotely)
  */
 PROXY_DECLARE(apr_status_t) ap_proxy_check_backend(const char *proxy_function,
                                                    proxy_conn_rec *conn,

diff --git a/modules/proxy/mod_proxy_ajp.c b/modules/proxy/mod_proxy_ajp.c
index 63bafe0216e26d242e97fc209a185b7e8c3e1ef1..a5222a6ef8eb800a265f620a5beb1e69ad7a71ef 100644 (file)
--- a/modules/proxy/mod_proxy_ajp.c
+++ b/modules/proxy/mod_proxy_ajp.c
@@ -783,8 +783,8 @@ static int proxy_ajp_handler(request_rec *r, proxy_worker *worker,
             break;
 
         /* Step Two: Make the Connection */
-        ap_proxy_check_backend(scheme, backend, r->server, 1);
-        if (ap_proxy_connect_backend(scheme, backend, worker, r->server)) {
+        if (ap_proxy_check_backend(scheme, backend, r->server, 1) &&
+                ap_proxy_connect_backend(scheme, backend, worker, r->server)) {
             ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(00896)
                           "failed to make connection to backend: %s",
                           backend->hostname);

diff --git a/modules/proxy/mod_proxy_fcgi.c b/modules/proxy/mod_proxy_fcgi.c
index cc22804bd6289cc15dbf4db4844b85d28e76f505..b0d338d12f59ce12acb9a4fa39fdf7134c4e7f61 100644 (file)
--- a/modules/proxy/mod_proxy_fcgi.c
+++ b/modules/proxy/mod_proxy_fcgi.c
@@ -935,12 +935,14 @@ static int proxy_fcgi_handler(request_rec *r, proxy_worker *worker,
      */  
     backend->close = 1;
     if (worker->s->disablereuse_set && !worker->s->disablereuse) { 
-        ap_proxy_check_backend(FCGI_SCHEME, backend, r->server, 1);
         backend->close = 0;
     }
 
     /* Step Two: Make the Connection */
-    if (ap_proxy_connect_backend(FCGI_SCHEME, backend, worker, r->server)) {
+    if ((backend->close || ap_proxy_check_backend(FCGI_SCHEME, backend,
+                                                  r->server, 1)) &&
+            ap_proxy_connect_backend(FCGI_SCHEME, backend, worker,
+                                     r->server)) {
         ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01079)
                       "failed to make connection to backend: %s",
                       backend->hostname);

diff --git a/modules/proxy/mod_proxy_http.c b/modules/proxy/mod_proxy_http.c
index a0d88eb05080383b9e8bfdd1d6261ba67db49c53..2519888d5678356e976250c1a8f584c5a1b7bc26 100644 (file)
--- a/modules/proxy/mod_proxy_http.c
+++ b/modules/proxy/mod_proxy_http.c
@@ -2069,8 +2069,9 @@ static int proxy_http_handler(request_rec *r, proxy_worker *worker,
         }
 
         /* Step Two: Make the Connection */
-        ap_proxy_check_backend(proxy_function, backend, r->server, 1);
-        if (ap_proxy_connect_backend(proxy_function, backend, worker, r->server)) {
+        if (ap_proxy_check_backend(proxy_function, backend, r->server, 1) &&
+                ap_proxy_connect_backend(proxy_function, backend, worker,
+                                         r->server)) {
             ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01114)
                           "HTTP: failed to make connection to backend: %s",
                           backend->hostname);

diff --git a/modules/proxy/proxy_util.c b/modules/proxy/proxy_util.c
index 1e00d8ab1a05723a370160861518577881823bab..7ae1d24392f5d5be977a7f441e184a4333c60df4 100644 (file)
--- a/modules/proxy/proxy_util.c
+++ b/modules/proxy/proxy_util.c
@@ -2704,43 +2704,49 @@ PROXY_DECLARE(apr_status_t) ap_proxy_check_backend(const char *proxy_function,
                                                    server_rec *s,
                                                    int expect_empty)
 {
-    apr_status_t rv;
-    
-    if (!conn->sock) {
-        return APR_ENOTSOCK;
-    }
+    apr_status_t rv = APR_SUCCESS;
+    proxy_worker *worker = conn->worker;
 
-    if (conn->connection) {
+    if (!PROXY_WORKER_IS_USABLE(worker)) {
+        /*
+         * 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.
+         */
+        rv = APR_NOTFOUND;
+    }
+    else if (!conn->sock) {
+        rv = APR_ENOTSOCK;
+    }
+    else if (conn->connection) {
         conn_rec *c = conn->connection;
         rv = ap_get_brigade(c->input_filters, conn->tmp_bb,
                             AP_MODE_SPECULATIVE, APR_NONBLOCK_READ, 1);
-        if (rv == APR_SUCCESS && expect_empty) {
+        if (APR_STATUS_IS_EAGAIN(rv)) {
+            rv = APR_SUCCESS;
+        }
+        else if (rv == APR_SUCCESS && expect_empty) {
             apr_off_t len = 0;
             apr_brigade_length(conn->tmp_bb, 0, &len);
             if (len) {
                 rv = APR_ENOTEMPTY;
             }
         }
-        else if (APR_STATUS_IS_EAGAIN(rv)) {
-            rv = APR_SUCCESS;
-        }
         apr_brigade_cleanup(conn->tmp_bb);
     }
-    else if (ap_proxy_is_socket_connected(conn->sock)) {
-        rv = APR_SUCCESS;
-    }
-    else {
+    else if (!ap_proxy_is_socket_connected(conn->sock)) {
         rv = APR_EPIPE;
     }
 
-    if (rv != APR_SUCCESS) {
+    if (rv != APR_SUCCESS && rv != APR_ENOTSOCK) {
         /* This clears conn->scpool (and associated data), so backup and
          * restore any ssl_hostname for this connection set earlier by
          * ap_proxy_determine_connection().
          */
         char ssl_hostname[PROXY_WORKER_RFC1035_NAME_SIZE];
-        if (!conn->ssl_hostname || PROXY_STRNCPY(ssl_hostname,
-                                                 conn->ssl_hostname)) {
+        if (rv == APR_NOTFOUND
+                || !conn->ssl_hostname
+                || PROXY_STRNCPY(ssl_hostname, conn->ssl_hostname)) {
             ssl_hostname[0] = '\0';
         }
 
@@ -2761,6 +2767,36 @@ PROXY_DECLARE(apr_status_t) ap_proxy_check_backend(const char *proxy_function,
         }
     }
 
+    if (rv != APR_NOTFOUND) {
+        /*
+         * Put the entire worker to error state if
+         * the PROXY_WORKER_IGNORE_ERRORS flag is not set.
+         * Although some connections may be alive
+         * no further connections to the worker could be made
+         */
+        if (rv != APR_SUCCESS && rv != APR_ENOTEMPTY) {
+            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)
+                    "ap_proxy_connect_backend disabling worker for (%s) for %"
+                    APR_TIME_T_FMT "s",
+                    worker->s->hostname, apr_time_sec(worker->s->retry));
+            }
+        }
+        else {
+            if (worker->s->retries) {
+                /*
+                 * A worker came back. So here is where we need to
+                 * either reset all params to initial conditions or
+                 * apply some sort of aging
+                 */
+            }
+            worker->s->error_time = 0;
+            worker->s->retries = 0;
+        }
+    }
+
     return rv;
 }
 
@@ -2770,7 +2806,6 @@ PROXY_DECLARE(int) ap_proxy_connect_backend(const char *proxy_function,
                                             server_rec *s)
 {
     apr_status_t rv;
-    int connected;
     int loglevel;
     apr_sockaddr_t *backend_addr = conn->addr;
     /* the local address to use for the outgoing connection */
@@ -2780,9 +2815,12 @@ PROXY_DECLARE(int) ap_proxy_connect_backend(const char *proxy_function,
     proxy_server_conf *conf =
         (proxy_server_conf *) ap_get_module_config(sconf, &proxy_module);
 
-    connected = (ap_proxy_check_backend(proxy_function, conn,
-                                        s, 0) == APR_SUCCESS);
-    while ((backend_addr || conn->uds_path) && !connected) {
+    rv = ap_proxy_check_backend(proxy_function, conn, s, 0);
+    if (rv == APR_NOTFOUND) {
+        return DECLINED;
+    }
+
+    while (rv != APR_SUCCESS && (backend_addr || conn->uds_path)) {
 #if APR_HAVE_SYS_UN_H
         if (conn->uds_path)
         {
@@ -2954,50 +2992,9 @@ 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.
-         * Although some connections may be alive
-         * no further connections to the worker could be made
-         */
-        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)
-                    "ap_proxy_connect_backend disabling worker for (%s) for %"
-                    APR_TIME_T_FMT "s",
-                    worker->s->hostname, apr_time_sec(worker->s->retry));
-            }
-        }
-        else {
-            if (worker->s->retries) {
-                /*
-                 * A worker came back. So here is where we need to
-                 * either reset all params to initial conditions or
-                 * apply some sort of aging
-                 */
-            }
-            worker->s->error_time = 0;
-            worker->s->retries = 0;
-        }
-        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;
     }
+
+    return rv == APR_SUCCESS ? OK : DECLINED;
 }
 
 static apr_status_t connection_shutdown(void *theconn)