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 Mon, 07 Oct 2013 16:49:54 GMT

Sorry to insist about this issue but I don't see how the current
mod_proxy_http's behaviour of connecting the backend (or checking
established connection reusability) before prefetching the client's body is
not a problem.

Pre-fetching 16K (or waiting for input filters to provide these) is not
always a fast operation, and the case where the backend closes its
connection in the meantime is easy to reproduce (and even likely to happen
with some applications).

Hence, why cannot mod_proxy_http prefetch the client's body *before* trying
anything with the backend connection, and only if it's all good, connect
(or reuse a connection to) the backend and then start forwarding the data
immediatly (flushing the prefetched ones) ?

By doing this, the time between the connect (or check) and the first bytes
sent to the backend is minimal.

It does not require to disable the prefetch since this one can really help
the decision about forwarding Content-Length vs chunked (which may not be
handled by the backend, IFAIK this is not a HTTP/1.1 requirement to handle
it for requests).

That was the purpose of the first patch I proposed ealier, but had no
feedback...
Maybe this message and the following patch could have more chance.

This patch is quite the same as the previous one (ap_proxy_http_request
split in 2, ap_proxy_http_prefetch to prefetch before connect and
ap_proxy_http_request to forward using the relevent
stream_reqbody_cl/chunked once prefetched and connected). It just fixes the
immediate flush of the prefetched bytes when it's time to forward, and the
handling of backend->close set by ap_proxy_create_hdrbrgd (or
ap_proxy_http_prefetch) before the connection even exist (or the previous
one to be reused).

Regards,
Yann.

Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c    (revision 1529127)
+++ modules/proxy/mod_proxy_http.c    (working copy)
@@ -251,6 +251,7 @@ static int stream_reqbody_chunked(apr_pool_t *p,
     while (!APR_BUCKET_IS_EOS(APR_BRIGADE_FIRST(input_brigade)))
     {
         char chunk_hdr[20];  /* must be here due to transient bucket. */
+        int flush = 0;

         /* If this brigade contains EOS, either stop or remove it. */
         if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(input_brigade))) {
@@ -299,13 +300,14 @@ static int stream_reqbody_chunked(apr_pool_t *p,
             }

             header_brigade = NULL;
+            flush = 1;
         }
         else {
             bb = input_brigade;
         }

         /* The request is flushed below this loop with chunk EOS header */
-        rv = ap_proxy_pass_brigade(bucket_alloc, r, p_conn, origin, bb, 0);
+        rv = ap_proxy_pass_brigade(bucket_alloc, r, p_conn, origin, bb,
flush);
         if (rv != OK) {
             return rv;
         }
@@ -389,12 +391,14 @@ static int stream_reqbody_cl(apr_pool_t *p,

     while (!APR_BUCKET_IS_EOS(APR_BRIGADE_FIRST(input_brigade)))
     {
+        int flush = 0;
+
         apr_brigade_length(input_brigade, 1, &bytes);
         bytes_streamed += bytes;

         /* If this brigade contains EOS, either stop or remove it. */
         if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(input_brigade))) {
-            seen_eos = 1;
+            seen_eos = flush = 1;

             /* We can't pass this EOS to the output_filters. */
             e = APR_BRIGADE_LAST(input_brigade);
@@ -444,13 +448,14 @@ static int stream_reqbody_cl(apr_pool_t *p,
             }

             header_brigade = NULL;
+            flush = 1;
         }
         else {
             bb = input_brigade;
         }

         /* Once we hit EOS, we are ready to flush. */
-        rv = ap_proxy_pass_brigade(bucket_alloc, r, p_conn, origin, bb,
seen_eos);
+        rv = ap_proxy_pass_brigade(bucket_alloc, r, p_conn, origin, bb,
flush);
         if (rv != OK) {
             return rv ;
         }
@@ -677,29 +682,33 @@ 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)
 {
     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;

     if (apr_table_get(r->subprocess_env, "force-proxy-request-1.0")) {
         if (r->expecting_100) {
@@ -710,17 +719,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 +738,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 +760,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 +871,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 +928,21 @@ skip_body:
         APR_BRIGADE_INSERT_TAIL(header_brigade, e);
     }

+    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 +954,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 +967,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 +1875,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 toclose = 0;
     /*
      * Use a shorter-lived pool to reduce memory usage
      * and avoid a memory leak
@@ -1924,16 +1951,55 @@ 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)) != OK) {
+        ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO()
+                      "HTTP: failed to prefetch the request body: %s",
+                      backend->hostname);
+        goto cleanup;
+    }
+
+    /* XXX: Reset backend->close now, since ap_proxy_http_prefetch sets it
to
+     * disable the reuse of the connection after this request (no
keep-alive),
+     * not to close any reusable connection before this request. However
assure
+     * what is expected later by using a local flag and do the right thing
when
+     * ap_proxy_connect_backend (below) provides the connection to close.
+     */
+    toclose = backend->close;
+    backend->close = 0;
+
     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)
@@ -1975,7 +2041,19 @@ static int proxy_http_handler(request_rec *r, prox
             }
         }

+        /* Don't recycle the connection if prefetch (above) told not to do
so */
+        if (toclose) {
+            backend->close = 1;
+            backend->connection->keepalive = AP_CONN_CLOSE;
+        }
+
         /* Step Three-and-a-Half: See if the socket is still connected (if
desired) */
+        /* XXX: ap_proxy_connect_backend aleady unconditionally checked
that
+         * before, if ap_proxy_connection_create was not called in the
meantime
+         * this check is redundant, otherwise can
ap_proxy_connection_create
+         * take so much time that a new check is relevant here? At least
this
+         * code should be run only when ap_proxy_connection_create is
called.
+         */
         if (worker->s->ping_timeout_set && worker->s->ping_timeout <
0 &&
             !ap_proxy_is_socket_connected(backend->sock)) {
             backend->close = 1;
@@ -1986,12 +2064,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;
Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c    (revision 1529127)
+++ 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));
[EOS]

Mime
View raw message