httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Luca Toscano <toscano.l...@gmail.com>
Subject Re: mod_proxy_fcgi, 304 responses and bogus error messages
Date Mon, 11 Jul 2016 21:33:10 GMT
2016-07-11 19:44 GMT+02:00 Jacob Champion <champion.p@gmail.com>:

> On 07/11/2016 08:53 AM, Luca Toscano wrote:
>
>> I am looking for some feedback about the patch proposed to figure out if
>> I am on the right track or not. Does it make sense to read all the data
>> returned by a FCGI backend even on error conditions to avoid subsequent
>> spurious reads or is there a smarter method?
>>
>
> (following up from IRC)
>
> Regarding your patch: I think that, rather than duplicate the jump back to
> recv_again, the error handling logic around the call to
> ap_scan_script_header_* should be improved. That function is documented to
> return binary success/failure (OK/INTERNAL_SERVER_ERROR), but in reality it
> can return other things too -- definitely NOT_MODIFIED (which is handled),
> perhaps PRECONDITION_FAILED (which is not handled), and maybe others?
>
> It looks like mod_proxy_fcgi used to take the binary approach ("anything
> that's not OK is an error; break out and run away") and when the exception
> was added for NOT_MODIFIED, that "error break" was left in. Perhaps we
> should just remove that break in the non-fatal-error cases (including,
> maybe, PRECONDITION_FAILED?).
>

I followed your suggestion and avoid the break in the HTTP_NOT_MODIFIED use
case, it works nicely! Updated the patch on the bugzilla PR. If anybody has
other suggestions please let us know..


> I don't know the correct answer to your "should we drain on error
> conditions too" question, since I don't know if the connection is torn down
> on errors. (If it's not torn down, then yeah, we should be draining the
> content and padding, but IMO tearing down the connection is probably the
> right thing to do.)
>

As we were discussing on IRC, the break were originally developed probably
for real error conditions and the HTTP_NOT_MODIFIED use case has been added
later on as a special use case. So I would be inclined to fix this
important use case without increasing the scope of the patch.


>
> And of course we need regression tests for all these fixes... does anyone
> have time to review
>
>     https://bz.apache.org/bugzilla/attachment.cgi?id=33867
>
> as a possible FCGI regression test template?
>
>
+1 for a good test suite around FCGI handlers, I'll try to extend/review it
during the next days.

Thanks!

Luca

Mime
View raw message