httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: svn commit: r1754732 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
Date Tue, 02 Aug 2016 23:50:11 GMT
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?

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).

Mime
View raw message