2016-07-11 19:44 GMT+02:00 Jacob Champion <[email protected]>: > 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
