Mailing List Archive

> The last hunk of r1877558 seems to be missing in this backport.
>
> @@ -2180,6 +2165,9 @@ static int proxy_http_handler(request_re
>
> /* Step Five: Receive the Response... Fall thru to cleanup */
> status = ap_proxy_http_process_response(req);
> + if (req->backend) {
> + proxy_run_detach_backend(r, req->backend);
> + }
>
> break;
> }
>
> I guess that it is not intentional and should go to 2.4.x as well.
> Anyone to confirm my supposition?

The hook does not exist in 2.4.x so I think it's OK.
The only reference to it is guarded with an MMN check to keep proxy_h2 common.

Le 18/04/2021 à 19:45, Eric Covener a écrit :
>> The last hunk of r1877558 seems to be missing in this backport.
>>
>> @@ -2180,6 +2165,9 @@ static int proxy_http_handler(request_re
>>
>> /* Step Five: Receive the Response... Fall thru to cleanup */
>> status = ap_proxy_http_process_response(req);
>> + if (req->backend) {
>> + proxy_run_detach_backend(r, req->backend);
>> + }
>>
>> break;
>> }
>>
>> I guess that it is not intentional and should go to 2.4.x as well.
>> Anyone to confirm my supposition?
>
> The hook does not exist in 2.4.x so I think it's OK.
> The only reference to it is guarded with an MMN check to keep proxy_h2 common.
>

+1.
Thanks for the clarification. (and sorry for the noise)

CJ

On 4/18/21 10:00 PM, Yann Ylavic wrote:

>
>> The last comment from Rüdiger seems to have been left un-answered.
>
> Ah yes, sorry Rüdiger, somehow I missed that one.
> First for 2.4.x it's true, if !ap_filter_should_yield() then
> ap_filter_output_pending() will always be DECLINED.
> But ap_filter_should_yield() and ap_filter_output_pending() in 2.4.x
> are just some helpers in proxy_util to keep trunk and 2.4.x code
> aligned, not the real util_filter functions.
>
> For trunk though, there is the ssl_io_filter_coalesce() case where
> !ap_filter_should_yield() does not mean that
> ap_filter_output_pending() has nothing to do. That's because
> ssl_io_filter_coalesce() does not play the
> ap_filter_{setaside,reinstate}_brigade() game for now, even though it
> potentially retains data.
> So in r1879416 I made a band aid such that ssl_io_filter_coalesce()
> releases its data when it's called from ap_filter_output_pending(),

Why should ap_filter_output_pending() call ssl_io_filter_coalesce?
As far as I see ssl_io_filter_coalesce does not get added to
the pending_output_filters ring and its private filter brigade would need
to be non empty to get called. Or is it called indirectly?

Regards

Rüdiger

On Tue, Apr 20, 2021 at 12:40 PM Ruediger Pluem <rpluem@apache.org> wrote:
>
> On 4/18/21 10:00 PM, Yann Ylavic wrote:
>
> >
> > For trunk though, there is the ssl_io_filter_coalesce() case where
> > !ap_filter_should_yield() does not mean that
> > ap_filter_output_pending() has nothing to do. That's because
> > ssl_io_filter_coalesce() does not play the
> > ap_filter_{setaside,reinstate}_brigade() game for now, even though it
> > potentially retains data.
> > So in r1879416 I made a band aid such that ssl_io_filter_coalesce()
> > releases its data when it's called from ap_filter_output_pending(),
>
> Why should ap_filter_output_pending() call ssl_io_filter_coalesce?
> As far as I see ssl_io_filter_coalesce does not get added to
> the pending_output_filters ring and its private filter brigade would need
> to be non empty to get called. Or is it called indirectly?

So I made ssl_io_filter_coalesce() enter the pending_output_filters
ring in r1889938 but it's still not enough because when it's called
with an empty brigade (like ap_filter_output_pending() does),
ssl_io_filter_coalesce() still retains its data.
I could special-case the empty brigade so that
ssl_io_filter_coalesce() releases everything, but this does not
address the tunneling loop case in mod_proxy where we shouldn't call
ap_filter_output_pending() if ap_filter_should_yield() already (or we
risk blocking).

So my plan now is to define a new bucket type (WC, for Write
Completion) and use it for both ap_filter_output_pending() (instead of
the empty brigade) and ap_proxy_transfer_between_connections(), to
tell coalescing/buffering filters that they should pass their data.
Any metadata bucket will do for ssl_io_filter_coalesce(), but the
FLUSH bucket is a bit too much (could make the core output filter
block) so there is no existing one to (ab)use AFAICT.

This is the attached patch, WDYT?

The WC bucket could also help reintroduce THRESHOLD_MIN_WRITE
(FlushMinThreshold) which was removed from the core output filter in
trunk because (I think) it defeated the write completion
(setaside/reinstate) mechanism. Not in this patch, but if the WC
bucket sounds like a good plan, it could be a follow up..

Regards;
Yann.