2016-08-02 8:22 GMT+02:00 Luca Toscano <[email protected]>: > Hi Yann, > > thanks a lot for the review, answer inline: > > 2016-08-02 1:03 GMT+02:00 Yann Ylavic <[email protected]>: > >> On Mon, Aug 1, 2016 at 12:55 PM, <[email protected]> wrote: >> > >> > Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c >> > URL: >> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c?rev=1754732&r1=1754731&r2=1754732&view=diff >> > >> ============================================================================== >> > --- httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c (original) >> > +++ httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c Mon Aug 1 >> 10:55:03 2016 >> > @@ -661,14 +661,26 @@ recv_again: >> > break; >> > } >> > else if (status == HTTP_NOT_MODIFIED) { >> > - /* The 304 response MUST NOT >> contain >> > - * a message-body, ignore it. >> > - * The break is not added since >> there might >> > - * be more bytes to read from the >> FCGI >> > - * connection. Even if the >> message-body is >> > - * ignored we want to avoid >> subsequent >> > - * bogus reads. */ >> > + /* A 304 response MUST NOT contain >> > + * a message-body, so we must >> ignore it but >> > + * some extra steps needs to be >> taken to >> > + * avoid inconsistencies. >> > + * The break is not added with >> connection >> > + * reuse set since there might be >> more bytes >> > + * to read from the FCGI >> connection, >> > + * like the message-body, >> >> There is no more to read for *this* request, it has its response: 304 >> (header only). > > If we read something that's for the next request (some response for no >> request), so we can discard it (conn->close=1 and done+break). >> But that'd better be checked just before reusing the connection >> (before sending the next request), the later detected the better; if >> anything read do not reuse (close) and establish a new one. > > >> > + * subsequent bogus reads (for >> example >> > + * the start of the message-body >> > + * interpreted as FCGI header). >> > + * With connecton reuse disabled >> (default) >> > + * we can safely break and force >> the end >> > + * of the FCGI processing phase >> since the >> > + * connection will be cleaned up >> later on. */ >> > ignore_body = 1; >> > + if (conn->close) { >> > + done = 1; >> > + break; >> > + } >> >> So this does not look correct to me, disablereuse or "connection: >> close" shouldn't control the behaviour, unless we close+break below by >> reading data with ignore_body == 1 (but that's too early IHMO). >> >> > So IIUC you are saying to always done+break in the 304 use case (to avoid > reading from the connection again), and then detect the response in another > place. Would you mind to give me some indication about where this check > should be? I am reviewing the code but it is not straightforward to me > where to make this change. >
With "detect the response" I meant the message-body of a response without a request :) Luca
