You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "William A. Rowe Jr." <wr...@rowe-clan.net> on 2013/12/12 00:34:11 UTC
Re: http_filter.c r1524770 open issue?
On Sat, 23 Nov 2013 19:10:21 +0100
Yann Ylavic <yl...@gmail.com> wrote:
> On Sat, Nov 23, 2013 at 6:52 PM, Yann Ylavic <yl...@gmail.com>
> wrote:
>
> > On Tue, Nov 19, 2013 at 3:27 PM, Yann Ylavic <yl...@gmail.com>
> > wrote:
> >
> >> On Mon, Nov 18, 2013 at 6:28 PM, William A. Rowe Jr.
> >> <wrowe@rowe-clan.net
> >> > wrote:
> >>
> >>>
> >>> 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);
> >> }
> >>
> >
> > Actually the shutdown would break SSL (which may need to write
> > during read, ~roughly~).
> > Maybe just closing the connection at the end of the response is
> > enough, which is assured by setting connection->keepalive to
> > AP_CONN_CLOSE here and ap_proxy_http_response() setting the
> > backend->close below in that case.
> >
>
> Forget about that (chicken and egg problem), the "end of the
> response" is the close on the backend side, so there is no way to
> forcibly do it, just trust...
No, the linger_close logic does all of the read-till-end logic. Yes,
closing the write end may cause the connection to forcibly terminate,
but we were already in the process of closing that connection. The
back end SSL should give up on their failure to read-before-write due
to the closed socket.
All we need to do is to return a 400-class error response to the client,
mark the backend connection closed, and close the write side of our
backend connection, letting the linger_close pump do the rest of our
work. That is, provided that the linger_close logic in the proxy
worker pool behaves as expected.
Re: http_filter.c r1524770 open issue?
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Dec 12, 2013 at 12:34 AM, William A. Rowe Jr.
<wr...@rowe-clan.net>wrote:
> On Sat, 23 Nov 2013 19:10:21 +0100
> Yann Ylavic <yl...@gmail.com> wrote:
>
> > On Sat, Nov 23, 2013 at 6:52 PM, Yann Ylavic <yl...@gmail.com>
> > wrote:
> >
> > > On Tue, Nov 19, 2013 at 3:27 PM, Yann Ylavic <yl...@gmail.com>
> > > wrote:
> > >
> > >> On Mon, Nov 18, 2013 at 6:28 PM, William A. Rowe Jr.
> > >> <wrowe@rowe-clan.net
> > >> > wrote:
> > >>
> > >>>
> > >>> 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);
> > >> }
> > >>
> > >
> > > Actually the shutdown would break SSL (which may need to write
> > > during read, ~roughly~).
> > > Maybe just closing the connection at the end of the response is
> > > enough, which is assured by setting connection->keepalive to
> > > AP_CONN_CLOSE here and ap_proxy_http_response() setting the
> > > backend->close below in that case.
> > >
> >
> > Forget about that (chicken and egg problem), the "end of the
> > response" is the close on the backend side, so there is no way to
> > forcibly do it, just trust...
>
> No, the linger_close logic does all of the read-till-end logic. Yes,
> closing the write end may cause the connection to forcibly terminate,
> but we were already in the process of closing that connection. The
> back end SSL should give up on their failure to read-before-write due
> to the closed socket.
>
We are indeed in the process of closing the connection, but aren't we
supposed to forward the response before?
Sorry to ramble on, but for this case, the RFC does not state the backend
must nor should use "Connection: close" to signify the peer it will be
(it's implicit enough), whereas it states proxies must do their best to
forward valid responses (I've no particular section to quote for that
statement though :) ).
>
> All we need to do is to return a 400-class error response to the client,
>
I guess you meant a 500-class error (the client isn't responsible for
anything here)
> mark the backend connection closed, and close the write side of our
> backend connection, letting the linger_close pump do the rest of our
> work. That is, provided that the linger_close logic in the proxy
> worker pool behaves as expected.
>
Why bother reading (more) a connection when finally we won't use anything
from it on the other side?
Just close it as soon as we don't see any C-L or chunked T-E or
"Connection: close" in the header, and respond with 5xx immediatly (note
this is not what I agree with, it just looks the same as your proposal).