httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: http_filter.c r1524770 open issue?
Date Tue, 19 Nov 2013 14:27:30 GMT
On Mon, Nov 18, 2013 at 6:28 PM, William A. Rowe Jr. <wrowe@rowe-clan.net>wrote:

> On Thu, 14 Nov 2013 00:22:55 +0100
> Yann Ylavic <ylavic.dev@gmail.com> wrote:
>
> > On Thu, Nov 14, 2013 at 12:05 AM, William A. Rowe Jr.
> > <wrowe@rowe-clan.net>wrote:
> >
> > > On Wed, 13 Nov 2013 17:14:15 -0500
> > > Jim Jagielski <jim@jaguNET.com> wrote:
> > >
> > > >
> > > > On Nov 13, 2013, at 2:25 AM, William A. Rowe Jr.
> > > > <wrowe@rowe-clan.net> wrote:
> > > > >
> > > > > Here we've unset C-L and T-E. but it makes no sense to wait if
> > > > > the origin server has no immediate plan to close the connection.
> > > >
> > > > I cannot grok the above. The RFC itself does not make
> > > > the differentiation between keepalive connections or not.
> > > > So what exactly is the issue? Are you saying we should
> > > > handle keepalive connections in this path differently?
> > > > How is that supported by the spec?
> > >
> > > Keep-alive (implicit in HTTP/1.1 absent a Connection: close header)
> > > is orthogonal to an unknown message body.  Think about it :)
> >
> > Regarding the connection persistance,
> >
> http://tools.ietf.org/id/draft-ietf-httpbis-p1-messaging-24.html#rfc.section.6.3states
> > :
> >
> >    In order to remain persistent, all messages on a connection MUST
> > have a self-defined message length (i.e., one not defined by closure
> > of the connection), as described in Section 3.3.
> >
> > So no keepalive is possible if the message length (content-coding) is
> > undefined.
>
> Correct.  It is NOT kept alive by the client (httpd) in this proxy case.
> But (to Jim's statement) the next-hop server claims it *is* kept alive
> and will handle the socket as kept-alive...
>

I don't see how the origin (next-hop) can claim the connection is kept
alive by using a *non* keepalive-able HTTP response, my understanding is
that the non-chunked T-E is something usable by the origin *but* in that
case it gives up keepalive-ability, and hence must close the connection at
the end of the request.


>
> > That could be a valid HTTP response :
> >
> > HTTP/1.1 200 OK
> > Transfer-Encoding: x-cleverness
> > Content-Length: 1000
> >
> > but still the C-L must be ignored by the receiver, and a
> > read-until-close be used by it.
>
> [Thank you for correcting me, the issue at hand is strictly with proxy
> response body processing in the first place.]
>
> Because the server claims it is keep-alive.  It will send its thousand
> bytes (decorated by whatever x-cleverness might frame it with), and will
> then enter a keepalive loop.  WE won't enter a keepalive loop, because
> we won't reuse the connection.  But the next-hop server is keeping it
> alive for whatever duration it believes it should be waiting for the
> next request from the client (httpd).  And in the interim, we are hung
> and holding our client's response open.  At best you can describe this
> as a waste of worker requests, and at worst, as a mild DoS.
>

See my first comment, I don't think the origin server is allowed to keep
its connection alive.

But indeed, if the origin is something not RFC compliant (or Alice),
mod_proxy has no way to know (just like with the no C-L/T-E
read-until-close case, the end of the response is the close), and the
scenario you describe above is clearly what happens.


>
> We need to perform a lingering close (reading until our read end was
> closed by the next-hop server).  The question is whether we tolerate the
> forwarding of keep-alive, other-than-'chunked' T-E proxy responses?
>

There is nothing like keep-alive forwarding with mod_proxy_http, currently
when the origin server's response is read-until-close, and HTTP/1.1 is used
(which is a T-E requirement too), the client's side response will be
chunked (as per ap_set_keepalive), and then can be kept alive regardless of
the origin server's connection state.


> By closing our write-end of the connection, we can signal to the server
> that we can't efficiently forward their response to the client (owing
> to the fact that the server believes this to be a keep-alive connection,
> and that we can't know where this specific response ends, until the
> server has given up on receiving another keep-alive request).
>

This would be a good way to ensure the connection is closed by the
origin, but half-closes are sometimes (and even often) mishandled, the
origin might consider the connection is fully closed (unwritable) when the
read-end is closed, that could be an issue too.

Otherwise, the following patch could do the trick :

Index: modules/http/http_filters.c
===================================================================
--- modules/http/http_filters.c    (revision 1541907)
+++ modules/http/http_filters.c    (working copy)
@@ -291,13 +291,19 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bu
          * denoted by C-L or T-E: chunked.
          *
          * Note that since the proxy uses this filter to handle the
-         * proxied *response*, proxy responses MUST be exempt.
+         * proxied *response*, proxy responses MUST be exempt, but
+         * ensure the connection is closed after the response.
          */
-        if (ctx->state == BODY_NONE && f->r->proxyreq !=
PROXYREQ_RESPONSE) {
-            e = apr_bucket_eos_create(f->c->bucket_alloc);
-            APR_BRIGADE_INSERT_TAIL(b, e);
-            ctx->eos_sent = 1;
-            return APR_SUCCESS;
+        if (ctx->state == BODY_NONE) {
+            if (f->r->proxyreq != PROXYREQ_RESPONSE) {
+                e = apr_bucket_eos_create(f->c->bucket_alloc);
+                APR_BRIGADE_INSERT_TAIL(b, e);
+                ctx->eos_sent = 1;
+                return APR_SUCCESS;
+            }
+            f->r->connection->keepalive = AP_CONN_CLOSE;
+            apr_socket_shutdown(ap_get_conn_socket(f->r->connection),
+                                APR_SHUTDOWN_WRITE);
         }

         /* Since we're about to read data, send 100-Continue if needed.
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c    (revision 1541907)
+++ modules/proxy/mod_proxy_http.c    (working copy)
@@ -1681,6 +1681,7 @@ int ap_proxy_http_process_response(apr_pool_t * p,
                         continue;
                     }
                     else if (rv == APR_EOF) {
+                        backend->close = 1;
                         break;
                     }
                     else if (rv != APR_SUCCESS) {
@@ -1825,6 +1826,11 @@ int ap_proxy_http_process_response(apr_pool_t * p,
         return DONE;
     }

+    /* If the underlying backend connection is to be closed, close it. */
+    if (origin->keepalive == AP_CONN_CLOSE) {
+        backend->close = 1;
+    }
+
     return OK;
 }

[END]


>
> My vote on this patch right now is -0.  I don't believe it must be
> applied to 2.4.x this moment for this coming release and would prefer
> that we alter the behavior as of a specific release, not incrementally
> across several different releases (e.g. 2.4.6 w/ old behavior, 2.4.7
> with an interim behavior and 2.4.8 with yet a third behavior).  But I
> won't block the patch further if it has the votes for backport now.
>
>
I don't try to push the patch in any specific version, you (and the httpd
team) only have the experience of this kind of change, and hence can take
the suitable decision. My attitude here is simply to give my 2cts about the
issue, which is that all the httpd branches (but trunk) currently do not
conform to the RFC/draft-httpbis...

Regards,
Yann.

Mime
View raw message