You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Yann Ylavic <yl...@gmail.com> on 2016/08/01 23:03:01 UTC
Re: svn commit: r1754732 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
On Mon, Aug 1, 2016 at 12:55 PM, <el...@apache.org> 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).
Regards,
Yann.
Re: svn commit: r1754732 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
Posted by Luca Toscano <to...@gmail.com>.
2016-08-02 19:18 GMT+02:00 Jacob Champion <ch...@gmail.com>:
> 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: svn commit: r1754732 -
/httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
Posted by Jacob Champion <ch...@gmail.com>.
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
Posted by Luca Toscano <to...@gmail.com>.
2016-08-03 1:50 GMT+02:00 Yann Ylavic <yl...@gmail.com>:
> On Tue, Aug 2, 2016 at 6:58 PM, Luca Toscano <to...@gmail.com>
> wrote:
> >
> > 2016-08-02 17:54 GMT+02:00 Yann Ylavic <yl...@gmail.com>:
> >>
> >> On Tue, Aug 2, 2016 at 5:05 PM, Yann Ylavic <yl...@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
Re: svn commit: r1754732 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Aug 2, 2016 at 6:58 PM, Luca Toscano <to...@gmail.com> wrote:
>
> 2016-08-02 17:54 GMT+02:00 Yann Ylavic <yl...@gmail.com>:
>>
>> On Tue, Aug 2, 2016 at 5:05 PM, Yann Ylavic <yl...@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).
Re: svn commit: r1754732 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
Posted by Luca Toscano <to...@gmail.com>.
2016-08-02 17:54 GMT+02:00 Yann Ylavic <yl...@gmail.com>:
> On Tue, Aug 2, 2016 at 5:05 PM, Yann Ylavic <yl...@gmail.com> 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
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Aug 2, 2016 at 5:05 PM, Yann Ylavic <yl...@gmail.com> 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...
Re: svn commit: r1754732 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Aug 2, 2016 at 4:07 PM, Luca Toscano <to...@gmail.com> wrote:
>
> 2016-08-02 15:23 GMT+02:00 Yann Ylavic <yl...@gmail.com>:
>>
>> 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
Posted by Luca Toscano <to...@gmail.com>.
2016-08-02 15:23 GMT+02:00 Yann Ylavic <yl...@gmail.com>:
> On Tue, Aug 2, 2016 at 2:59 PM, Luca Toscano <to...@gmail.com>
> wrote:
> >
> > 2016-08-02 10:48 GMT+02:00 Yann Ylavic <yl...@gmail.com>:
> >>
> >> 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
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Aug 2, 2016 at 2:59 PM, Luca Toscano <to...@gmail.com> wrote:
>
> 2016-08-02 10:48 GMT+02:00 Yann Ylavic <yl...@gmail.com>:
>>
>> 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
Posted by Luca Toscano <to...@gmail.com>.
2016-08-02 10:48 GMT+02:00 Yann Ylavic <yl...@gmail.com>:
> On Tue, Aug 2, 2016 at 8:22 AM, Luca Toscano <to...@gmail.com>
> 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
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Aug 2, 2016 at 8:22 AM, Luca Toscano <to...@gmail.com> 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).
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.
Re: svn commit: r1754732 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
Posted by Luca Toscano <to...@gmail.com>.
2016-08-02 8:22 GMT+02:00 Luca Toscano <to...@gmail.com>:
> Hi Yann,
>
> thanks a lot for the review, answer inline:
>
> 2016-08-02 1:03 GMT+02:00 Yann Ylavic <yl...@gmail.com>:
>
>> On Mon, Aug 1, 2016 at 12:55 PM, <el...@apache.org> 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
Re: svn commit: r1754732 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
Posted by Luca Toscano <to...@gmail.com>.
Hi Yann,
thanks a lot for the review, answer inline:
2016-08-02 1:03 GMT+02:00 Yann Ylavic <yl...@gmail.com>:
> On Mon, Aug 1, 2016 at 12:55 PM, <el...@apache.org> 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.
Regards,
Luca