httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: mod_proxy, oooled backend connections and the keep-alive race condition
Date Wed, 02 Oct 2013 22:14:39 GMT
Helo,

as proposed in a previous message, this new patch that splits
ap_proxy_http_request() in two, the prefetch part being now
ap_proxy_http_prefetch().

This allows to prefetch (partially, as is) the request body before
connecting the backend, and connect or check whether the connection is
still alive just before sending the first data, reducing the race to the
minimum, even if the client or an input filter retains that data.

The approach is quite naive and can surely be improved, the local vars from
the original ap_proxy_http_request() now shared between the two are
provided as output parameters by ap_proxy_http_prefetch() and consumed as
input parameters by ap_proxy_http_request().

The formerly local header/input brigades are now created by
proxy_http_handler (the caller) and used by both ap_proxy_http_prefetch()
and ap_proxy_http_request().

header_brigade used to be backend->connection->bucket_alloc()ated, it is
now r->connection->bucket_alloc()ated since there not always a
backend->connection at that time. I don't know if that's a problem (leak?).

At least it works here...

Regards,
Yann.

Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c    (revision 1528615)
+++ modules/proxy/proxy_util.c    (working copy)
@@ -3092,7 +3092,9 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
         buf = apr_pstrcat(p, r->method, " ", url, " HTTP/1.1" CRLF, NULL);
     }
     if (apr_table_get(r->subprocess_env, "proxy-nokeepalive")) {
-        origin->keepalive = AP_CONN_CLOSE;
+        if (origin) {
+            origin->keepalive = AP_CONN_CLOSE;
+        }
         p_conn->close = 1;
     }
     ap_xlate_proto_to_ascii(buf, strlen(buf));
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c    (revision 1528615)
+++ modules/proxy/mod_proxy_http.c    (working copy)
@@ -677,30 +677,41 @@ static apr_status_t proxy_buckets_lifetime_transfo
     return rv;
 }

+enum rb_methods {
+    RB_INIT,
+    RB_STREAM_CL,
+    RB_STREAM_CHUNKED,
+    RB_SPOOL_CL
+};
+
 static
-int ap_proxy_http_request(apr_pool_t *p, request_rec *r,
+int ap_proxy_http_prefetch(apr_pool_t *p, request_rec *r,
                                    proxy_conn_rec *p_conn, proxy_worker
*worker,
                                    proxy_server_conf *conf,
                                    apr_uri_t *uri,
-                                   char *url, char *server_portstr)
+                                   char *url, char *server_portstr,
+                                   apr_bucket_brigade *header_brigade,
+                                   apr_bucket_brigade *input_brigade,
+                                   char **old_cl_val, char **old_te_val,
+                                   enum rb_methods *rb_method, int
*keepalive)
 {
     conn_rec *c = r->connection;
     apr_bucket_alloc_t *bucket_alloc = c->bucket_alloc;
-    apr_bucket_brigade *header_brigade;
-    apr_bucket_brigade *input_brigade;
     apr_bucket_brigade *temp_brigade;
     apr_bucket *e;
     char *buf;
     apr_status_t status;
-    enum rb_methods {RB_INIT, RB_STREAM_CL, RB_STREAM_CHUNKED,
RB_SPOOL_CL};
-    enum rb_methods rb_method = RB_INIT;
-    char *old_cl_val = NULL;
-    char *old_te_val = NULL;
     apr_off_t bytes_read = 0;
     apr_off_t bytes;
     int force10, rv;
-    conn_rec *origin = p_conn->connection;

+    /* XXX: This somehow should be fixed in the API.
+     * Assumes p_conn->close is 0 here since ap_proxy_create_hdrbrgd and
code
+     * below will use it to disable keep-alive and not for the connection
to
+     * be closed before reuse.
+     */
+    AP_DEBUG_ASSERT(!p_conn->close);
+
     if (apr_table_get(r->subprocess_env, "force-proxy-request-1.0")) {
         if (r->expecting_100) {
             return HTTP_EXPECTATION_FAILED;
@@ -710,17 +721,13 @@ static
         force10 = 0;
     }

-    header_brigade = apr_brigade_create(p, origin->bucket_alloc);
     rv = ap_proxy_create_hdrbrgd(p, header_brigade, r, p_conn,
                                  worker, conf, uri, url, server_portstr,
-                                 &old_cl_val, &old_te_val);
+                                 old_cl_val, old_te_val);
     if (rv != OK) {
         return rv;
     }

-    /* We have headers, let's figure out our request body... */
-    input_brigade = apr_brigade_create(p, bucket_alloc);
-
     /* sub-requests never use keepalives, and mustn't pass request bodies.
      * Because the new logic looks at input_brigade, we will self-terminate
      * input_brigade and jump past all of the request body logic...
@@ -733,15 +740,15 @@ static
     if (!r->kept_body && r->main) {
         /* XXX: Why DON'T sub-requests use keepalives? */
         p_conn->close = 1;
-        if (old_cl_val) {
-            old_cl_val = NULL;
+        if (*old_cl_val) {
+            *old_cl_val = NULL;
             apr_table_unset(r->headers_in, "Content-Length");
         }
-        if (old_te_val) {
-            old_te_val = NULL;
+        if (*old_te_val) {
+            *old_te_val = NULL;
             apr_table_unset(r->headers_in, "Transfer-Encoding");
         }
-        rb_method = RB_STREAM_CL;
+        *rb_method = RB_STREAM_CL;
         e = apr_bucket_eos_create(input_brigade->bucket_alloc);
         APR_BRIGADE_INSERT_TAIL(input_brigade, e);
         goto skip_body;
@@ -755,20 +762,19 @@ static
      * encoding has been done by the extensions' handler, and
      * do not modify add_te_chunked's logic
      */
-    if (old_te_val && strcasecmp(old_te_val, "chunked") != 0) {
+    if (*old_te_val && strcasecmp(*old_te_val, "chunked") != 0) {
         ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01093)
                       "%s Transfer-Encoding is not supported", old_te_val);
         return HTTP_INTERNAL_SERVER_ERROR;
     }

-    if (old_cl_val && old_te_val) {
+    if (*old_cl_val && *old_te_val) {
         ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01094)
                       "client %s (%s) requested Transfer-Encoding "
                       "chunked body with Content-Length (C-L ignored)",
                       c->client_ip, c->remote_host ? c->remote_host: "");
         apr_table_unset(r->headers_in, "Content-Length");
-        old_cl_val = NULL;
-        origin->keepalive = AP_CONN_CLOSE;
+        *old_cl_val = NULL;
         p_conn->close = 1;
     }

@@ -867,42 +873,42 @@ static
          * If we expected no body, and read no body, do not set
          * the Content-Length.
          */
-        if (old_cl_val || old_te_val || bytes_read) {
-            old_cl_val = apr_off_t_toa(r->pool, bytes_read);
+        if (*old_cl_val || *old_te_val || bytes_read) {
+            *old_cl_val = apr_off_t_toa(r->pool, bytes_read);
         }
-        rb_method = RB_STREAM_CL;
+        *rb_method = RB_STREAM_CL;
     }
-    else if (old_te_val) {
+    else if (*old_te_val) {
         if (force10
              || (apr_table_get(r->subprocess_env, "proxy-sendcl")
                   && !apr_table_get(r->subprocess_env, "proxy-sendchunks")
                   && !apr_table_get(r->subprocess_env,
"proxy-sendchunked"))) {
-            rb_method = RB_SPOOL_CL;
+            *rb_method = RB_SPOOL_CL;
         }
         else {
-            rb_method = RB_STREAM_CHUNKED;
+            *rb_method = RB_STREAM_CHUNKED;
         }
     }
-    else if (old_cl_val) {
+    else if (*old_cl_val) {
         if (r->input_filters == r->proto_input_filters) {
-            rb_method = RB_STREAM_CL;
+            *rb_method = RB_STREAM_CL;
         }
         else if (!force10
                   && (apr_table_get(r->subprocess_env, "proxy-sendchunks")
                       || apr_table_get(r->subprocess_env,
"proxy-sendchunked"))
                   && !apr_table_get(r->subprocess_env, "proxy-sendcl")) {
-            rb_method = RB_STREAM_CHUNKED;
+            *rb_method = RB_STREAM_CHUNKED;
         }
         else {
-            rb_method = RB_SPOOL_CL;
+            *rb_method = RB_SPOOL_CL;
         }
     }
     else {
         /* This is an appropriate default; very efficient for no-body
          * requests, and has the behavior that it will not add any C-L
-         * when the old_cl_val is NULL.
+         * when the *old_cl_val is NULL.
          */
-        rb_method = RB_SPOOL_CL;
+        *rb_method = RB_SPOOL_CL;
     }

 /* Yes I hate gotos.  This is the subrequest shortcut */
@@ -924,6 +930,28 @@ skip_body:
         APR_BRIGADE_INSERT_TAIL(header_brigade, e);
     }

+    /* XXX: This somehow should be fixed in the API (see beginning
comment).
+     * backend->close is set to disable keepalive, no need to close the
socket
+     * until further notice, let the caller know about that though.
+     */
+    *keepalive = !p_conn->close;
+    p_conn->close = 0;
+
+    return OK;
+}
+
+static
+int ap_proxy_http_request(apr_pool_t *p, request_rec *r,
+                                   proxy_conn_rec *p_conn,
+                                   apr_bucket_brigade *header_brigade,
+                                   apr_bucket_brigade *input_brigade,
+                                   char *old_cl_val, char *old_te_val,
+                                   enum rb_methods rb_method)
+{
+    int rv;
+    apr_off_t bytes_read = 0;
+    conn_rec *origin = p_conn->connection;
+
     /* send the request body, if any. */
     switch(rb_method) {
     case RB_STREAM_CHUNKED:
@@ -935,6 +963,7 @@ skip_body:
                                    input_brigade, old_cl_val);
         break;
     case RB_SPOOL_CL:
+        apr_brigade_length(input_brigade, 1, &bytes_read);
         rv = spool_reqbody_cl(p, r, p_conn, origin, header_brigade,
                                   input_brigade, (old_cl_val != NULL)
                                               || (old_te_val != NULL)
@@ -947,6 +976,7 @@ skip_body:
     }

     if (rv != OK) {
+        conn_rec *c = r->connection;
         /* apr_status_t value has been logged in lower level method */
         ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01097)
                       "pass request body failed to %pI (%s) from %s (%s)",
@@ -1854,10 +1884,16 @@ static int proxy_http_handler(request_rec *r, prox
     char *scheme;
     const char *proxy_function;
     const char *u;
+    apr_bucket_brigade *header_brigade, *header_bb;
+    apr_bucket_brigade *input_brigade, *input_bb;
     proxy_conn_rec *backend = NULL;
     int is_ssl = 0;
     conn_rec *c = r->connection;
     int retry = 0;
+    char *old_cl_val = NULL, *old_te_val = NULL;
+    enum rb_methods rb_method = RB_INIT;
+    char *locurl = url;
+    int keepalive = 1;
     /*
      * Use a shorter-lived pool to reduce memory usage
      * and avoid a memory leak
@@ -1924,16 +1960,47 @@ static int proxy_http_handler(request_rec *r, prox
         backend->close = 1;
     }

+    /* Step One: Determine Who To Connect To */
+    if ((status = ap_proxy_determine_connection(p, r, conf, worker,
backend,
+                                            uri, &locurl, proxyname,
+                                            proxyport, server_portstr,
+                                            sizeof(server_portstr))) != OK)
+        goto cleanup;
+
+
+    /* Step Once: Prefetch (partially) the request body so to increase the
+     * chances to get whole (or enough) body and determine Content-Length
vs
+     * chunked or spool. By doing this before connecting or reusing a
backend
+     * connection, minimize the delay between checking whether this
connection
+     * is still alive and the first packet sent, should the client be slow
or
+     * some input filter retain the data.
+     */
+    input_brigade = apr_brigade_create(p, c->bucket_alloc);
+    header_brigade = apr_brigade_create(p, c->bucket_alloc);
+    if ((status = ap_proxy_http_prefetch(p, r, backend, worker, conf, uri,
+                    locurl, server_portstr, header_brigade, input_brigade,
+                    &old_cl_val, &old_te_val, &rb_method, &keepalive)) !=
OK) {
+        ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO()
+                      "HTTP: failed to prefetch the request body: %s",
+                      backend->hostname);
+        goto cleanup;
+    }
+
     while (retry < 2) {
-        char *locurl = url;
+        if (retry) {
+            char *newurl = url;
+            /* Step One-Retry: Redetermine Who To Connect To */
+            if ((status = ap_proxy_determine_connection(p, r, conf, worker,
+                            backend, uri, &newurl, proxyname, proxyport,
+                            server_portstr, sizeof(server_portstr))) != OK)
+                break;
+            /* XXX: the code assumes locurl is not changed during the
loops,
+             * or ap_proxy_http_prefetch would have to be called every
time,
+             * and header_brigade changed accordingly...
+             */
+            AP_DEBUG_ASSERT(!strcmp(newurl, locurl));
+        }

-        /* Step One: Determine Who To Connect To */
-        if ((status = ap_proxy_determine_connection(p, r, conf, worker,
backend,
-                                                uri, &locurl, proxyname,
-                                                proxyport, server_portstr,
-                                                sizeof(server_portstr)))
!= OK)
-            break;
-
         /* Step Two: Make the Connection */
         if (ap_proxy_connect_backend(proxy_function, backend, worker,
r->server)) {
             ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01114)
@@ -1974,6 +2041,9 @@ static int proxy_http_handler(request_rec *r, prox
                               ssl_hostname);
             }
         }
+        if (!keepalive) {
+            backend->connection->keepalive = AP_CONN_CLOSE;
+        }

         /* Step Three-and-a-Half: See if the socket is still connected (if
desired) */
         if (worker->s->ping_timeout_set && worker->s->ping_timeout <
0 &&
@@ -1986,12 +2056,18 @@ static int proxy_http_handler(request_rec *r, prox
             continue;
         }

+        /* Preserve the header/input brigades since they may be retried */
+        input_bb = apr_brigade_create(p,
backend->connection->bucket_alloc);
+        header_bb = apr_brigade_create(p,
backend->connection->bucket_alloc);
+        proxy_buckets_lifetime_transform(r, input_brigade, input_bb);
+        proxy_buckets_lifetime_transform(r, header_brigade, header_bb);
+
         /* Step Four: Send the Request
          * On the off-chance that we forced a 100-Continue as a
          * kinda HTTP ping test, allow for retries
          */
-        if ((status = ap_proxy_http_request(p, r, backend, worker,
-                                        conf, uri, locurl,
server_portstr)) != OK) {
+        if ((status = ap_proxy_http_request(p, r, backend, header_bb,
input_bb,
+                        old_cl_val, old_te_val, rb_method)) != OK) {
             if ((status == HTTP_SERVICE_UNAVAILABLE) &&
worker->s->ping_timeout_set &&
                  worker->s->ping_timeout > 0) {
                 backend->close = 1;
[EOS]



On Tue, Oct 1, 2013 at 2:36 PM, Plüm, Rüdiger, Vodafone Group <
ruediger.pluem@vodafone.com> wrote:

>
>
> > -----Ursprüngliche Nachricht-----
> > Von: Joe Orton
> > Gesendet: Dienstag, 1. Oktober 2013 14:23
> > An: Plüm, Rüdiger, Vodafone Group
> > Cc: dev@httpd.apache.org
> > Betreff: Re: mod_proxy, oooled backend connections and the keep-alive
> > race condition
> >
> > On Fri, Aug 02, 2013 at 12:33:58PM +0000, Plüm, Rüdiger, Vodafone Group
> > wrote:
> > > The typical way to solve this today is to know the keepalive timeout
> > > of the backend and set ttl for this worker to a value a few seconds
> > > below.
> >
> > I just looked at a case where the user is hitting this problem and does
> > already have the ttl & keepalive-timeout configured like that; and a few
> > seconds difference is not necessarily enough.  The problem is that the
> > "prefetch" of the 16K request body is making it a lot worse - the worst
> > case is (16K/packet size) * Timeout seconds to complete the "prefetch".
>
> True.
>
> >
> > That's time when the proxy *thinks* the connection is valid but the
> > backend thinks the connection is idle.  And in most reverse-proxy cases
> > that prefetch is adding basically no value AFAICT - the backend is a
> > known vintage and probably HTTP/1.1.  So... could we make the prefetch
> > stuff configurable away?
>
> IMHO no issue with this. Let's hear what others say.
> I guess the main point of prefetch was to make better decisions whether to
> use chunked
> encoding when sending to the backend. Or provide a CL to the backend when
> the real client does not.
>
> Regards
>
> Rüdiger
>
>

Mime
View raw message