On 3/31/20 1:19 AM, Yann Ylavic wrote:
> On Mon, Mar 30, 2020 at 10:23 PM Ruediger Pluem <[email protected]> wrote:
>>
>> On 3/26/20 2:00 PM, Yann Ylavic wrote:
>>>
>
> []
>>> + ap_pass_brigade(c->output_filters, bb);
>>
>> But if request filters still have setaside brigades and these would be
>> reactivated by ap_run_output_pending(c); in the
>> WRITE_COMPLETION phase they would write their response content after the EOR
>> bucket to the filter chain, which actually mean that
>> the EOR bucket is in the middle of the response data.
>
> Yes that's the issue from the start, request filters are not supposed
> to retain anything after the EOS bucket, they are simply not called
> anymore once they return.
Although this is not needed to fix the issue that caused the thread it is IMHO
a pity. This means we fix the case of a large file
/ morphing bucket causing blocking writes over an SSL connection, but the same
thing through a gzip / mod_substitute /
mod_proxy_html filter still causes the blocking writes. But probably one step
after another.
>
>> We would need to ensure that EOR is sent after EOS. But I guess we cannot
>> sent EOR through the remaining request filters as most
>> of them ignore and swallow what they see after EOS.
>
> No, too dangerous IMHO. If we pass the EOR to request filters they
> need to be really cautious about what do with it, and even more after
> they pass it (don't control it anymore). Anything they have allocated
> on r->pool (temporary brigade or whatever) can't be used anymore once
> the EOR is destroyed.
>
> The EOR has to be sent to the connection filters, when we know that
> the request is finished, and before we start sending anything for the
> next request. That's ap_process_request_after_handler() currently and
> it looks like the right place to me.
An idea would be a to have a filter that comes after all request filters
(either as very first connection filter or in the same
position as the the ap_request_core_filter and as soon as it sees an EOS bucket
it adds an EOR bucket afterwards.
For sync MPM's we would need an option to make ap_run_output_pending flush all
data at least on request filter level.
>
>
> FWIW, attached is v2 with simpler "batching" in
> ap_filter_setaside_brigade(), no spurious hunk, bb cleaned up at the
> end, and APR_ENOTIMPL when called from a filter < AP_FTYPE_CONNECTION.
Looks good to me. I understood that you verified that it fixes the issue and I
see no obvious downsides right now.
Let's go with it and improve (feature wise) from there.
Regards
Rüdiger
