Re: svn commit: r1754732 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
2016-08-02 10:48 GMT+02:00 Yann Ylavic: > On Tue, Aug 2, 2016 at 8:22 AM, Luca Toscano > wrote: > > > > 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. > > Yes, any following data is for the next request. > > > 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. > > IMHO it belongs in ap_proxy_is_socket_connected(), see r1750392 (and > follow up r1750474). > Nice! I didn't remember about this new functionality and didn't think about using it for my use case. > If this is in place (trunk only for now), we can simply done+break on > 204 or 304 in mod_proxy_fcgi I'd prefer to think about this change as a near-future backport proposal since it is really annoying for people using mod_proxy_fcgi in big production environments (bogus 503s and entries in the error logs for 304s all the time). I tried the following patch http://apaste.info/qgK (that afaiu should leverage the code that you pointed out) and tested again my use case http://apaste.info/n6V (contains php test script, httpd proxy conf and curl requests) getting a weird result, namely an alternation of 304 and 200 status codes (as opposed to 304 all the times). From the logs ( http://apaste.info/jhF) it seems that the second curl request reads the whole response rather than discarding it. Should this be already part of ap_proxy_is_socket_connected or is it still to be added? My relatively new experience with proxy_util.c does not help a lot :) > What I don't know is whether or not we need to read AP_FCGI_END_REQUEST > anyway? > If that's the case, we should indeed not break until then, and > conn->close=1+done+break if we read anything else before. > But wouldn't this be like reading the whole response (headers + message-body) and then discard what not needed? I am not getting how would we get to the AP_FCGI_END_REQUEST bytes without reading the other ones. Thanks a lot! Regards, Luca
Re: svn commit: r1754732 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
On Tue, Aug 2, 2016 at 2:59 PM, Luca Toscanowrote: > > 2016-08-02 10:48 GMT+02:00 Yann Ylavic : >> >> What I don't know is whether or not we need to read AP_FCGI_END_REQUEST >> anyway? >> If that's the case, we should indeed not break until then, and >> conn->close=1+done+break if we read anything else before. > > But wouldn't this be like reading the whole response (headers + > message-body) and then discard what not needed? I am not getting how would > we get to the AP_FCGI_END_REQUEST bytes without reading the other ones. Actually I don't know if (but I now suppose that) the end of the response for the current current is marked by AP_FCGI_END_REQUEST, irrespective of the HTTP status code. If that's correct, we indeed shouldn't break until we got AP_FCGI_END_REQUEST, so that we can reuse the connection when all goes well. But since it's a 304 (or a 204 which should be handled the same here), we don't expect any body between the header and AP_FCGI_END_REQUEST, so if that happens (something else than AP_FCGI_END_REQUEST is read while ignore_body == 1), we must bail out with conn->close=1 to avoid reusing the connection for any further request. If the response header has not been forwarded already, we could either respond with a BAD_GATEWAY instead or forward it before leaving (possibly be lenient with CGIs). In any case we must not forward any body bytes to the client (with 304/204) or reuse this out of sync connection with the backend later. Regards, Yann.
Re: svn commit: r1754732 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
2016-08-02 15:23 GMT+02:00 Yann Ylavic: > On Tue, Aug 2, 2016 at 2:59 PM, Luca Toscano > wrote: > > > > 2016-08-02 10:48 GMT+02:00 Yann Ylavic : > >> > >> What I don't know is whether or not we need to read AP_FCGI_END_REQUEST > >> anyway? > >> If that's the case, we should indeed not break until then, and > >> conn->close=1+done+break if we read anything else before. > > > > But wouldn't this be like reading the whole response (headers + > > message-body) and then discard what not needed? I am not getting how > would > > we get to the AP_FCGI_END_REQUEST bytes without reading the other ones. > > Actually I don't know if (but I now suppose that) the end of the > response for the current current is marked by AP_FCGI_END_REQUEST, > irrespective of the HTTP status code. > I will do some research but afaik it should be always present. Any other opinion from the list? > If that's correct, we indeed shouldn't break until we got > AP_FCGI_END_REQUEST, so that we can reuse the connection when all goes > well. > This was exactly what I way trying to do with this commit, but only when the connection is reused. It has an annoying downside, like introducing a bit of latency between the HTTP headers flush to the client and the end of the request for the use case described below (but it works well). > But since it's a 304 (or a 204 which should be handled the same here), > we don't expect any body between the header and AP_FCGI_END_REQUEST, > so if that happens (something else than AP_FCGI_END_REQUEST is read > while ignore_body == 1), we must bail out with conn->close=1 to avoid > reusing the connection for any further request. > If the response header has not been forwarded already, we could either > respond with a BAD_GATEWAY instead or forward it before leaving > (possibly be lenient with CGIs). > There is one case in which a mod_proxy_fcgi returns a 304 but there is also body to read/process, namely when a "regular" FCGI response is returned by a backend script with a Last-Modified header and the client uses something like If-Modified-Since. In this case ap_scan_script_header_err_brigade_ex (through ap_scan_script_header_err_core_ex -> ap_meets_conditions) sets the "status" variable in mod_proxy_fcgi to HTTP_NOT_MODIFIED, but the message-body has been sent anyway. I am not sure if the FCGI script would be in violation of the RFC behaving like this but if not it is a valid use case. In any case we must not forward any body bytes to the client (with > 304/204) or reuse this out of sync connection with the backend later. > +1, I agree 100% Regards, Luca
Re: svn commit: r1754732 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
On Tue, Aug 2, 2016 at 4:07 PM, Luca Toscanowrote: > > 2016-08-02 15:23 GMT+02:00 Yann Ylavic : >> >> If that's correct, we indeed shouldn't break until we got >> AP_FCGI_END_REQUEST, so that we can reuse the connection when all goes >> well. > > This was exactly what I way trying to do with this commit, but only when the > connection is reused. It has an annoying downside, like introducing a bit of > latency between the HTTP headers flush to the client and the end of the > request for the use case described below (but it works well). If I understand well the below, even if we don't reuse the connection we should not close it under the backend when we turn a response to a 304. > >> >> But since it's a 304 (or a 204 which should be handled the same here), >> we don't expect any body between the header and AP_FCGI_END_REQUEST, >> so if that happens (something else than AP_FCGI_END_REQUEST is read >> while ignore_body == 1), we must bail out with conn->close=1 to avoid >> reusing the connection for any further request. >> If the response header has not been forwarded already, we could either >> respond with a BAD_GATEWAY instead or forward it before leaving >> (possibly be lenient with CGIs). > > There is one case in which a mod_proxy_fcgi returns a 304 but there is also > body to read/process, namely when a "regular" FCGI response is returned by a > backend script with a Last-Modified header and the client uses something > like If-Modified-Since. In this case ap_scan_script_header_err_brigade_ex > (through ap_scan_script_header_err_core_ex -> ap_meets_conditions) sets the > "status" variable in mod_proxy_fcgi to HTTP_NOT_MODIFIED, OK, I missed that. > but the > message-body has been sent anyway. No, nothing is sent to the client yet (!seen_end_of_headers). > I am not sure if the FCGI script would be > in violation of the RFC behaving like this but if not it is a valid use > case. The CGI does nothing special, it just does not handle pre-conditions and httpd will do that for it. So we need to detect whether the 304 is a CGI Status or ours. It seems that in the former case r->status is 304, whereas in the latter case this is the local variable 'status' only. We could possibly set "cgi_status = r->status" in mod_proxy_fcgi and bail out if anything is read but AP_FCGI_END_REQUEST when cgi_status == [23]04. Otherwise let ignore_body suck up the response body (if any) so that the connection can be reused if all goes well until the next request.
Re: svn commit: r1754732 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
On Tue, Aug 2, 2016 at 6:58 PM, Luca Toscanowrote: > > 2016-08-02 17:54 GMT+02:00 Yann Ylavic : >> >> On Tue, Aug 2, 2016 at 5:05 PM, Yann Ylavic 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).
Re: svn commit: r1754732 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
On Tue, Aug 2, 2016 at 5:05 PM, Yann Ylavicwrote: > > So we need to detect whether the 304 is a CGI Status or ours. > It seems that in the former case r->status is 304, whereas in the > latter case this is the local variable 'status' only. > We could possibly set "cgi_status = r->status" in mod_proxy_fcgi and > bail out if anything is read but AP_FCGI_END_REQUEST when cgi_status > == [23]04. > Otherwise let ignore_body suck up the response body (if any) so that > the connection can be reused if all goes well until the next request. 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. Enforcing that Status 204/304 has no body is not part of this bugfix I guess...
Re: svn commit: r1754732 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
2016-08-02 17:54 GMT+02:00 Yann Ylavic: > On Tue, Aug 2, 2016 at 5:05 PM, Yann Ylavic wrote: > > > > So we need to detect whether the 304 is a CGI Status or ours. > > It seems that in the former case r->status is 304, whereas in the > > latter case this is the local variable 'status' only. > > We could possibly set "cgi_status = r->status" in mod_proxy_fcgi and > > bail out if anything is read but AP_FCGI_END_REQUEST when cgi_status > > == [23]04. > > Otherwise let ignore_body suck up the response body (if any) so that > > the connection can be reused if all goes well until the next request. > > 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. > > Enforcing that Status 204/304 has no body is not part of this bugfix I > guess... > So basically keeping only http://svn.apache.org/r1752347 that avoids to break before AP_FCGI_END_REQUEST ? 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, 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). If this is ok I can revert r1754732 and leave my backport proposal as it is :) Thanks for the patience! Luca
Re: svn commit: r1754732 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
To follow up on an IRC comment: I like performance improvements, but this is code that we've identified a large number of bugs/misfeatures in recently, and I think there are plans for further changes soon. Performance improvements tend to fragment things and make later refactoring difficult, so I'd (mildly) prefer that they be delayed just a little bit, until we implement/backport those other changes. --Jacob
Re: svn commit: r1754732 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
2016-08-02 19:18 GMT+02:00 Jacob Champion: > To follow up on an IRC comment: > > I like performance improvements, but this is code that we've identified a > large number of bugs/misfeatures in recently, and I think there are plans > for further changes soon. Performance improvements tend to fragment things > and make later refactoring difficult, so I'd (mildly) prefer that they be > delayed just a little bit, until we implement/backport those other changes. > > --Jacob > Should be all reverted in http://svn.apache.org/r1754983. Thanks a lot to Yann and Jacob for the follow up! Luca
Re: Anyone using CLion ?
On Mon, Aug 1, 2016 at 7:24 AM, Jim Jagielskiwrote: > CLion is a C IDE built around cmake. I thought I'd try using it > w/ trunk but have had numerous issues, likely because our cmake > implementation is a work-in-progress. Anyone have success in > using CLion on httpd or, in fact, any non-cmake C project? > Other than the win32 build, I haven't revisited the cmake implementation. It's probably simplest to get the gnu make build target working first, then any of the Eclipse or CLion targets should 'just work'.
Re: 2.4.24 soon?
On Fri, Jul 22, 2016 at 5:10 AM, Jim Jagielskiwrote: > I think we should look into other stuff we could fold in in > the short term. > Seems overdue for us to fold the HTTP_STRICT logic back into 2.4 and 2.2 before we tag and roll again. It seems pretty odd not to follow RFC2068, never mind the RFC 2616 and 723x group of specs. Objections or other observations? Cheers, Bill
Re: 2.4.24 soon?
On Aug 2, 2016 11:58 AM, "William A Rowe Jr"wrote: > > On Fri, Jul 22, 2016 at 5:10 AM, Jim Jagielski wrote: >> >> I think we should look into other stuff we could fold in in >> the short term. > > > Seems overdue for us to fold the HTTP_STRICT logic back into 2.4 and 2.2 > before we tag and roll again. It seems pretty odd not to follow RFC2068, > never mind the RFC 2616 and 723x group of specs. Objections or other > observations? One additional thought... On 2.2 and 2.4 I see this change as entirely opt-in, no disruption to a user performing a subversion upgrade. On 2.6/3.0 I'd want us to seriously consider changing the out-of-the-box default to strict parsing.
Re: 2.4.24 soon?
On 08/02/2016 11:12 AM, William A Rowe Jr wrote: One additional thought... On 2.2 and 2.4 I see this change as entirely opt-in, no disruption to a user performing a subversion upgrade. On 2.6/3.0 I'd want us to seriously consider changing the out-of-the-box default to strict parsing. +1. (I have no strong opinions on whether or not this should go into the next release, though.) --Jacob
Re: svn commit: r1754732 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
Hi Yann, thanks a lot for the review, answer inline: 2016-08-02 1:03 GMT+02:00 Yann Ylavic: > On Mon, Aug 1, 2016 at 12:55 PM, 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=1754731=1754732=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. Regards, Luca
Re: svn commit: r1754732 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
2016-08-02 8:22 GMT+02:00 Luca Toscano: > Hi Yann, > > thanks a lot for the review, answer inline: > > 2016-08-02 1:03 GMT+02:00 Yann Ylavic : > >> On Mon, Aug 1, 2016 at 12:55 PM, 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=1754731=1754732=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
Re: svn commit: r1750953 - /httpd/httpd/trunk/server/util_script.c
2016-08-01 21:13 GMT+02:00 Jacob Champion> > > As stated above, this is not my first choice -- but I wouldn't oppose it > if that's what the consensus comes to. > > else if (!ap_cstr_casecmp(w, "Last-Modified")) { >> -apr_time_t parsed_date = apr_date_parse_rfc(l); >> +apr_time_t parsed_date = apr_date_parse_http(l); >> > > apr_date_parse_http() is not good enough; IIUC, it completely ignores > timezones, which further corrupts non-GMT Last-Modified stamps. We either > want strict parsing or actual correction, not something in the middle. You are right, but this is the current behavior, this is why I used apr_date_parse_http. My idea was to keep what we have at the moment, adding a clear log that states the inconsistency found by httpd and why it has been corrected. In this case it would simply mean that a value considered "in the future" from GMT would be logged and corrected accordingly. This would still leave out other wrong use cases like a datetime string in the US/West timezone "in the future" if converted in GMT, but to solve it we'd need to interpret the Last-Modified header value with the apr_date_parse_rfc function and afaiu we still need to reach an agreement :) So I am really in favor of using apr_date_parse_rfc but I wanted to come up with a possible to solution to satisfy everybody and help our users with some clue. Example about the wrong use case mentioned by me for everybody (it helped me a lot so it might also be useful to other people reading): === HTTP/1.1 200 OK Date: Tue, 02 Aug 2016 07:54:39 GMT Server: Apache/2.5.0-dev (Unix) Last-Modified: Tue, 02 Aug 2016 00:54:39 GMT Content-Type: text/html; charset=UTF-8 Last-Modified sent: Tue, 02 Aug 2016 00:54:39 -0700 Now in GMT: Tue, 02 Aug 2016 07:54:39 + === As you can see, the FCGI script sets a Last-Modified header value now() in the "America/Los_Angeles" timezone, but it gets interpreted by httpd as a GMT datestring "in the past". So for example "now() + 2 hours" in the "America/Los_Angeles" timezone would still get interpreted as "in the past" by httpd, avoiding any datetime correction to GMT now(). Same example with the code change that we have originally proposed: === HTTP/1.1 200 OK Date: Tue, 02 Aug 2016 08:08:14 GMT Server: Apache/2.5.0-dev (Unix) Last-Modified: Tue, 02 Aug 2016 08:08:14 GMT Content-Type: text/html; charset=UTF-8 Last-Modified sent: Tue, 02 Aug 2016 01:08:14 -0700 Now in GMT: Tue, 02 Aug 2016 08:08:14 + === The debate is all around, as far as I have understood, to if httpd should be able to do the above "interpretation" or not. 2016-08-01 23:13 GMT+02:00 Jacob Champion : > On 08/01/2016 01:35 PM, William A Rowe Jr wrote: > >> So this alternative is not my first choice. Invalid headers should >> really either be corrected (if the correction is obvious, safe, and >> helpful), or dropped entirely. Or the entire response should be >> 500'd, but we run into major compatibility breaks if we choose that >> route. >> >> No, I agree that a 500 is not an option. Drop it, or fix it loudly in >> the logs, >> but we can't break existing deployments (which don't accept non-GMT >> dates today, FWIW). >> > > Based on conversations with Luca, existing deployments do "accept" non-GMT > dates, silently corrupting the value, due to the use of the *_parse_http() > function. > > I'm happy to react to bad input following a parsable date string, e.g. any >> non-"GMT" signifier, as entirely bad input worth emitting to the error >> log. >> Let's help CGI authors find their bugs instead of generating unexpected >> results, such as 5-8 hour wrong "Last Modified" dates in the US and >> now() throughout Europe, owing to the time zone deltas. >> > > Cool. I'm not opposed to loudly dropping non-GMT timestamps (though I > still prefer fixing them up). Wouldn't this introduce a breaking change? I know that everybody is supposed to look into the changelog before upgrading httpd but as we often see this is not the case and we could trigger nasty issues in various installations imho. Compared to this solution I'd much prefer to go for the proposed one, invasive but less disruptive :) As Jacob said, more opinions are very welcome! One general comment: I really like discussions but in this case I feel that we would have solved the issue in 20 minutes talking in person :) So maybe a conversation on IRC whenever everybody has a bit of time could help reaching a final consensus? Luca
Re: svn commit: r1754732 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
On Tue, Aug 2, 2016 at 8:22 AM, Luca Toscanowrote: > > 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. Yes, any following data is for the next request. > 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. IMHO it belongs in ap_proxy_is_socket_connected(), see r1750392 (and follow up r1750474). If this is in place (trunk only for now), we can simply done+break on 204 or 304 in mod_proxy_fcgi. What I don't know is whether or not we need to read AP_FCGI_END_REQUEST anyway? If that's the case, we should indeed not break until then, and conn->close=1+done+break if we read anything else before.