Re: proposed fix/filter to pause reqtimeout in bewteen requests...
On Fri, Mar 4, 2016 at 5:00 PM, <[email protected]> wrote:
> https://bz.apache.org/bugzilla/show_bug.cgi?id=59045
>
> --- Comment #13 from Ruediger Pluem <[email protected]> ---
> (In reply to Yann Ylavic from comment #12)
>> (In reply to Ruediger Pluem from comment #11)
>> >
>> > Hm. Is another output filter really the best solution? It has to be passed
>> > for every run of the output chain.
>> > How about setting a note to c->notes in check_pipeline that informs
>> > mod_reqtimeout to step out of our way temporarily? This removes the need
>> > for
>> > mod_reqtimeout to second guess from other conditions that it has to step
>> > out.
>>
>> It's not really an heavy filter, not sure it performs slower than an
>> apr_table lookup...
>> This filter shouldn't be noticeable from a performance POV, and I think it
>> is sane in any case, do we (will) control all the speculative/non-blocking
>> (administrative) reads between requests?
>
> I admit that it might be a matter of taste. You can argue that you don't know
> where all these actions between requests happen and hence that in these cases
> the callers that pull from the input chain miss to set the note. Furthermore
> you can argue that the callers shouldn't need to know about mod_reqtimeout and
> the need to disable it and that they might forget to enable it again or
> disable
> it again if we do so automatically in mod_reqtimeout.
I've nothing more to argue :)
> On the other hand we had
> a fix for this issue already in 2.4.16 and did not capture all cases.
I hesitate for the final, provided the filter solution works and is
accepted, to remove the leading check:
if (block == APR_NONBLOCK_READ && mode == AP_MODE_SPECULATIVE) {
/* The source of these above us in the core is check_pipeline(), which
* is between requests but before this filter knows to reset timeouts
* during pre_read_request(). If they appear elsewhere, just don't
* check or extend the time since they won't block and we'll see the
* bytes again later
*/
return ap_get_brigade(f->next, bb, mode, block, readbytes);
}
in reqtimeout_filter().
It would indeed be redundant now, and may miss other (relevant)
speculative reads that could trigger a timeout.
Below there is another check on AP_MODE_SPECULATIVE which prevents
updating the timer until the real (non speculative) bytes are read,
this one would remain.
The goal is to timeout as soon as we can...
Thoughts?
> Probably
> we missed other edge cases and need to add further more less complex code to
> detect them in mod_reqtimeout whereas in the notes case we would just need to
> set the note on callers side.
We'd need to add these same missed cases on the callers side too.
But as I said I'm not opposed to both solutions, the filter for the
core case and a note for the modules' convenience (couldn't we rearm
mod_reqtimeout on mod_[proxy_]http2 and configurably on
mod_proxy_wstunnel with this? Looks sensible to me...).
Regards,
Yann.
