On Tue, Mar 31, 2020 at 7:35 AM Ruediger Pluem <rpl...@apache.org> wrote: > > On 3/31/20 1:19 AM, Yann Ylavic wrote: > > > > 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.
Yes, if we don't want to block we need the handler to handle should_yield or EAGAIN or whatever, SUSPEND (or register an MPM callback) and be prepared to be called again. I don't see how we could do it automagically in the core for the handler. > > > >> 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. This would potentially segfault a lot of output filters / handlers IMHO, what if we add EOR just after EOS, pass the whole to the core output filter which fails to write to the socket (ECONNRESET). The EOR gets destroyed, we return the error to the request filters/handlers which then shouldn't touch anything r->pool allocated. This includes the bb they used to pass their buckets for instance, so the usual ap_pass_brigade(r->output_filters, bb) + apr_brigade_cleanup(bb) would simply crash if bb = apr_brigade_create(r->pool, c->bucket_alloc) initially. > > > > > > > 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. Thanks, r1875947. Regards, Yann.
