You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Luca Toscano <to...@gmail.com> on 2016/09/07 18:25:22 UTC
Feedback for mod_proxy_fcgi and 412 precondition failures
Hi Apache devs,
as follow up of http://svn.apache.org/r1752347 I discovered that simple GET
requests with the right If-UnModified-Since header trigger the same
problem, namely message-body bytes left unread (and errors logged
incorrectly) due to HTTP_PRECONDITION_FAILURE not handled. The 412 in this
use case is returned by ap_meets_conditions, called via
ap_scan_script_header_err_brigade_ex.
Since the EOS bucket is sent anyway if status != OK, I came up with the
following patch:
Index: modules/proxy/mod_proxy_fcgi.c
===================================================================
--- modules/proxy/mod_proxy_fcgi.c (revision 1759650)
+++ modules/proxy/mod_proxy_fcgi.c (working copy)
@@ -670,7 +670,7 @@
* bogus reads. */
ignore_body = 1;
}
- else {
+ else if (status !=
HTTP_PRECONDITION_FAILED) {
ap_log_rerror(APLOG_MARK, APLOG_ERR,
0, r, APLOGNO(01070)
"Error parsing script
headers");
rv = APR_EINVAL;
I am still confused if this is the right approach or not, more specifically:
1) Do we need to return a message-body in this use case? Maybe handling
HTTP_PRECONDITION_FAILURE before the EOS bucket is sent?
2) Should the HTTP_PRECONDITION_FAILED case be coupled with
HTTP_NOT_MODIFIED (so leveraging the ignore_body = 1) ? IIUC it shouldn't
change much..
Thanks!
Regards,
Luca
Re: Feedback for mod_proxy_fcgi and 412 precondition failures
Posted by Luca Toscano <to...@gmail.com>.
2016-09-07 22:41 GMT+02:00 Jacob Champion <ch...@gmail.com>:
> On 09/07/2016 11:25 AM, Luca Toscano wrote:
> > Index: modules/proxy/mod_proxy_fcgi.c
> > ===================================================================
> > --- modules/proxy/mod_proxy_fcgi.c (revision 1759650)
> > +++ modules/proxy/mod_proxy_fcgi.c (working copy)
> > @@ -670,7 +670,7 @@
> > * bogus reads. */
> > ignore_body = 1;
> > }
> > - else {
> > + else if (status !=
> HTTP_PRECONDITION_FAILED) {
> > ap_log_rerror(APLOG_MARK,
> APLOG_ERR, 0, r, APLOGNO(01070)
> > "Error parsing
> script headers");
> > rv = APR_EINVAL;
> >
>
> I can't find the last conversation we had over this in my outbox and my
> memory has holes; sorry if I repeat anything that was already covered a
> while back.
>
Thanks for following up :)
>
> I'd prefer not to patch this with another special case, but fix the
> architecture instead. The ap_scan_script_header_xxx functions are
> documented to be pass/fail, but they're actually tri-state: pass, hard
> failure (5xx), or conditional "failure". It seems like the recent bugs
> associated with this code are due to interactions with this third state.
>
> Ideally, we wouldn't be doing conditional logic in the function that is
> documented to be a dumb header parser, but if we can't change that due to
> compatibility concerns, we should at least adjust calling code to
> understand the tri-state. In other words: OK = pass, 5xx = fail, anything
> else is potentially a conditional failure that we might need to handle
> specially.
>
> 1) Do we need to return a message-body in this use case? Maybe handling
>> HTTP_PRECONDITION_FAILURE before the EOS bucket is sent?
>>
>
> Since this "optimization" only happens for GET/HEAD, I think we should
> scrap the response body.
>
> 2) Should the HTTP_PRECONDITION_FAILED case be coupled with
>> HTTP_NOT_MODIFIED (so leveraging the ignore_body = 1) ? IIUC it
>> shouldn't change much..
>>
>
> Makes sense to me. If *all* the conditional failure codes (are there any
> others?) can be treated in the same way, that would be a good way to handle
> the "third state" logic.
>
+1
>
> (Tangent for a separate conversation: how do we feel about the fact that
> this optimization still makes the backend do the same amount of work to
> generate the response? Would it be better for us to abort the FCGI stream
> once we figure out we're not going to use it?)
My two cents: it could be an option, some code for a similar issue (client
disconnects) has been written in
https://bz.apache.org/bugzilla/show_bug.cgi?id=56188 and could be taken as
example. It would be a very delicate change though, so not sure if it is
worth the risk of introducing unexpected bugs.
Luca
Re: Feedback for mod_proxy_fcgi and 412 precondition failures
Posted by Jacob Champion <ch...@gmail.com>.
On 09/07/2016 11:25 AM, Luca Toscano wrote:
> Index: modules/proxy/mod_proxy_fcgi.c
> ===================================================================
> --- modules/proxy/mod_proxy_fcgi.c (revision 1759650)
> +++ modules/proxy/mod_proxy_fcgi.c (working copy)
> @@ -670,7 +670,7 @@
> * bogus reads. */
> ignore_body = 1;
> }
> - else {
> + else if (status !=
HTTP_PRECONDITION_FAILED) {
> ap_log_rerror(APLOG_MARK,
APLOG_ERR, 0, r, APLOGNO(01070)
> "Error parsing
script headers");
> rv = APR_EINVAL;
>
I can't find the last conversation we had over this in my outbox and my
memory has holes; sorry if I repeat anything that was already covered a
while back.
I'd prefer not to patch this with another special case, but fix the
architecture instead. The ap_scan_script_header_xxx functions are
documented to be pass/fail, but they're actually tri-state: pass, hard
failure (5xx), or conditional "failure". It seems like the recent bugs
associated with this code are due to interactions with this third state.
Ideally, we wouldn't be doing conditional logic in the function that is
documented to be a dumb header parser, but if we can't change that due
to compatibility concerns, we should at least adjust calling code to
understand the tri-state. In other words: OK = pass, 5xx = fail,
anything else is potentially a conditional failure that we might need to
handle specially.
> 1) Do we need to return a message-body in this use case? Maybe handling
> HTTP_PRECONDITION_FAILURE before the EOS bucket is sent?
Since this "optimization" only happens for GET/HEAD, I think we should
scrap the response body.
> 2) Should the HTTP_PRECONDITION_FAILED case be coupled with
> HTTP_NOT_MODIFIED (so leveraging the ignore_body = 1) ? IIUC it
> shouldn't change much..
Makes sense to me. If *all* the conditional failure codes (are there any
others?) can be treated in the same way, that would be a good way to
handle the "third state" logic.
(Tangent for a separate conversation: how do we feel about the fact that
this optimization still makes the backend do the same amount of work to
generate the response? Would it be better for us to abort the FCGI
stream once we figure out we're not going to use it?)
--Jacob