httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Luca Toscano <toscano.l...@gmail.com>
Subject Re: svn commit: r1754732 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
Date Wed, 03 Aug 2016 09:54:37 GMT
2016-08-03 1:50 GMT+02:00 Yann Ylavic <ylavic.dev@gmail.com>:

> On Tue, Aug 2, 2016 at 6:58 PM, Luca Toscano <toscano.luca@gmail.com>
> wrote:
> >
> > 2016-08-02 17:54 GMT+02:00 Yann Ylavic <ylavic.dev@gmail.com>:
> >>
> >> On Tue, Aug 2, 2016 at 5:05 PM, Yann Ylavic <ylavic.dev@gmail.com>
> wrote:
> >>
> >> Actually, unless we want to check/enforce that a Status 304 (as
> >> opposed to a 304 set by ap_meets_conditions) is not followed by a
> >> body, the correct behaviour is probably just to revert this commit
> >> (r1754732).
> >>
> >> We already ignore the body (when we ought to) until
> >> AP_FCGI_END_REQUEST, and I see no reason to close the connection
> >> underneath the backend if we turn a 200 to a 304, this connection can
> >> even be reused if all goes well.
> >
> > So basically keeping only http://svn.apache.org/r1752347 that avoids to
> > break before AP_FCGI_END_REQUEST ?
>
> Yes, I think it was the right fix already, thanks Luca.
>
> > The only downside that I see is that in
> > case a FCGI response turns up into a 304 (the 'status = 304' use case
> > mentioned earlier) then the HTTP headers are shipped to the external
> client
> > very quickly,
>
> Yes, but we also shouldn't close (even when not reusing) before having
> read the end or the backend may consider its response/transaction is
> incomplete (turning into 304 should be transparent on both sides).
>
>
> > but then some latency is added because httpd needs to read all
> > the bytes from the FCGI connection before completing the HTTP response
> (at
> > least this is what I have observed in my tests).
>
> There is no latency from the client (thanks to EOS), right?
> Or would we need a FLUSH?
>

I re-studied a bit the difference between EOS/EOR and reviewed my tests.
The only use case in which I see a big "delay" between headers and end of
request on the client side is when I use telnet, but not with other tools
like curl, so I have been probably fooled by it. As you mentioned below the
delay should not be visible to the external client but only to httpd (since
the thread will still be busy).


> But yes, the thread may still be busy after AP_FCGI_END_REQUEST
> (done), with a last call to get_data_full() before leaving.
>
> I think we should let this read for the next request, so how about:
>
> Index: modules/proxy/mod_proxy_fcgi.c
> ===================================================================
> --- modules/proxy/mod_proxy_fcgi.c    (revision 1755008)
> +++ modules/proxy/mod_proxy_fcgi.c    (working copy)
> @@ -445,7 +445,7 @@ static apr_status_t dispatch(proxy_conn_rec *conn,
>                               int *bad_request, int *has_responded)
>  {
>      apr_bucket_brigade *ib, *ob;
> -    int seen_end_of_headers = 0, done = 0, ignore_body = 0;
> +    int seen_end_of_headers = 0, ignore_body = 0;
>      apr_status_t rv = APR_SUCCESS;
>      int script_error_status = HTTP_OK;
>      conn_rec *c = r->connection;
> @@ -472,7 +472,7 @@ static apr_status_t dispatch(proxy_conn_rec *conn,
>      ib = apr_brigade_create(r->pool, c->bucket_alloc);
>      ob = apr_brigade_create(r->pool, c->bucket_alloc);
>
> -    while (! done) {
> +    while (1) {
>          apr_interval_time_t timeout;
>          apr_size_t len;
>          int n;
> @@ -772,7 +772,7 @@ recv_again:
>                  break;
>
>              case AP_FCGI_END_REQUEST:
> -                done = 1;
> +                /* we are done below */
>                  break;
>
>              default:
> @@ -780,8 +780,8 @@ recv_again:
>                                "Got bogus record %d", type);
>                  break;
>              }
> -            /* Leave on above switch's inner error. */
> -            if (rv != APR_SUCCESS) {
> +            /* Leave on above switch's inner end/error. */
> +            if (rv != APR_SUCCESS || type == AP_FCGI_END_REQUEST) {
>                  break;
>              }
>
> ?
>
> The final EOR will do its work faster, and we'll be noticed of
> spurious data on next request (r1750474 series).
>

I tested the patch and it works fine in my testing environment, together
with r1750474 it could be a good improvement.

Thanks a lot for the long email thread and all the pointers!

Regards,

Luca

Mime
View raw message