On Tue, Aug 2, 2016 at 6:58 PM, Luca Toscano <[email protected]> wrote:
>
> 2016-08-02 17:54 GMT+02:00 Yann Ylavic <[email protected]>:
>>
>> On Tue, Aug 2, 2016 at 5:05 PM, Yann Ylavic <[email protected]> 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).
