Mailing List Archive

On Tue, Feb 2, 2021 at 10:32 AM Ruediger Pluem <rpluem@apache.org> wrote:
>
> > New Revision: 1885605
> >
> > URL: http://svn.apache.org/viewvc?rev=1885605&view=rev
[]
> > + /* Yield if the output filters stack is full? This is to avoid
> > + * blocking and give the caller a chance to POLLOUT async.
> > + */
> > + if (flags & AP_PROXY_TRANSFER_YIELD_PENDING) {
> > + int rc = OK;
> > +
> > + if (!ap_filter_should_yield(c_o->output_filters)) {
> > + rc = ap_filter_output_pending(c_o);
>
> I am confused here: !ap_filter_should_yield(c_o->output_filters) means there is no pending output.
> Why should we try to send it then? Shouldn't it be the other way round?

Yes, !ap_filter_should_yield() means that there is no pending output,
but only for filters that play the
ap_filter_{setaside,reinstate}_brigade() game.

The goal here is to try to flush pending data of filters that don't
ap_filter_setaside_brigade(), like e.g. ssl_io_filter_coalesce() which
is not using setaside/reinstate (but is kind of aware of them still,
see r1879416, I tried to move it to using setaside/resinstate but the
result is more complicated than the original code, so I passed on that
patch for now and pushed the one-liner..).

But (and this is why the code is like this), we don't want to try to
flush those potential non-setaside pending data if there are setaside
pending data already (like in the core output filter), otherwise we
might block in some filter (like the core filter still) if given more
data while already at the limits.

This poll()ing loop really depends on staying in the POLLOUT state
until there are no more pending data (setaside or not), otherwise we
"risk" the blocking call soon or later, at least with the current "no
EAGAIN" on output mechanism. Hope this clarifies why it's done like
this..

Regards;
Yann.

On 2/2/21 4:18 PM, Yann Ylavic wrote:
> On Tue, Feb 2, 2021 at 10:32 AM Ruediger Pluem <rpluem@apache.org> wrote:
>>
>>> New Revision: 1885605
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1885605&view=rev
> []
>>> + /* Yield if the output filters stack is full? This is to avoid
>>> + * blocking and give the caller a chance to POLLOUT async.
>>> + */
>>> + if (flags & AP_PROXY_TRANSFER_YIELD_PENDING) {
>>> + int rc = OK;
>>> +
>>> + if (!ap_filter_should_yield(c_o->output_filters)) {
>>> + rc = ap_filter_output_pending(c_o);
>>
>> I am confused here: !ap_filter_should_yield(c_o->output_filters) means there is no pending output.
>> Why should we try to send it then? Shouldn't it be the other way round?
>
> Yes, !ap_filter_should_yield() means that there is no pending output,
> but only for filters that play the
> ap_filter_{setaside,reinstate}_brigade() game.
>
> The goal here is to try to flush pending data of filters that don't
> ap_filter_setaside_brigade(), like e.g. ssl_io_filter_coalesce() which

But isn't ap_filter_output_pending a noop that always returns DECLINED in case
of !ap_filter_should_yield(c_o->output_filters)?

Regards

Rüdiger

Le 03/02/2021 à 09:24, Ruediger Pluem a écrit :
>
>
> On 2/2/21 4:18 PM, Yann Ylavic wrote:
>> On Tue, Feb 2, 2021 at 10:32 AM Ruediger Pluem <rpluem@apache.org> wrote:
>>>
>>>> New Revision: 1885605
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1885605&view=rev
>> []
>>>> + /* Yield if the output filters stack is full? This is to avoid
>>>> + * blocking and give the caller a chance to POLLOUT async.
>>>> + */
>>>> + if (flags & AP_PROXY_TRANSFER_YIELD_PENDING) {
>>>> + int rc = OK;
>>>> +
>>>> + if (!ap_filter_should_yield(c_o->output_filters)) {
>>>> + rc = ap_filter_output_pending(c_o);
>>>
>>> I am confused here: !ap_filter_should_yield(c_o->output_filters) means there is no pending output.
>>> Why should we try to send it then? Shouldn't it be the other way round?
>>
>> Yes, !ap_filter_should_yield() means that there is no pending output,
>> but only for filters that play the
>> ap_filter_{setaside,reinstate}_brigade() game.
>>
>> The goal here is to try to flush pending data of filters that don't
>> ap_filter_setaside_brigade(), like e.g. ssl_io_filter_coalesce() which
>
> But isn't ap_filter_output_pending a noop that always returns DECLINED in case
> of !ap_filter_should_yield(c_o->output_filters)?
>
> Regards
>
> Rüdiger
>

Same backport for which a just sent a question about a potential missing
part of r1877558.


Unrelated, but is there still something to discuss here.
The last comment from Rüdiger seems to have been left un-answered.

CJ

On Sun, Apr 18, 2021 at 7:37 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> Le 03/02/2021 à 09:24, Ruediger Pluem a écrit :
> >
> > On 2/2/21 4:18 PM, Yann Ylavic wrote:
> >> On Tue, Feb 2, 2021 at 10:32 AM Ruediger Pluem <rpluem@apache.org> wrote:
> >>>
> >>>> New Revision: 1885605
> >>>>
> >>>> URL: http://svn.apache.org/viewvc?rev=1885605&view=rev
> >> []
> >>>> + /* Yield if the output filters stack is full? This is to avoid
> >>>> + * blocking and give the caller a chance to POLLOUT async.
> >>>> + */
> >>>> + if (flags & AP_PROXY_TRANSFER_YIELD_PENDING) {
> >>>> + int rc = OK;
> >>>> +
> >>>> + if (!ap_filter_should_yield(c_o->output_filters)) {
> >>>> + rc = ap_filter_output_pending(c_o);
> >>>
> >>> I am confused here: !ap_filter_should_yield(c_o->output_filters) means there is no pending output.
> >>> Why should we try to send it then? Shouldn't it be the other way round?
> >>
> >> Yes, !ap_filter_should_yield() means that there is no pending output,
> >> but only for filters that play the
> >> ap_filter_{setaside,reinstate}_brigade() game.
> >>
> >> The goal here is to try to flush pending data of filters that don't
> >> ap_filter_setaside_brigade(), like e.g. ssl_io_filter_coalesce() which
> >
> > But isn't ap_filter_output_pending a noop that always returns DECLINED in case
> > of !ap_filter_should_yield(c_o->output_filters)?
>
> Same backport for which a just sent a question about a potential missing
> part of r1877558.

Like Eric said, this is not relevant for 2.4.

>
> Unrelated, but is there still something to discuss here.

Probably :)

> 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(),
but the real fix would be that mod_ssl coalesces the data using
apr_brigade_write() and save/restore them with
ap_filter_{setaside,reinstate}_brigade(). But I didn't finish that
patch yet..

Note that in 2.4.x ssl_io_filter_coalesce() in not an issue for the
mod_proxy tunneling loop because ap_proxy_tunnel_create() initially
does:
/* No coalescing filters */
ap_remove_output_filter_byhandle(c_i->output_filters,
"SSL/TLS Coalescing Filter");
ap_remove_output_filter_byhandle(c_o->output_filters,
"SSL/TLS Coalescing Filter");
But in trunk we don't do that anymore, because ultimately
ap_proxy_tunnel_run() could be used for full async mod_proxy(_http),
not only tunneling, meaning that all the filters (and not only
connection ones) need to remain in place. I'm working on that (slowly)
too..


Hopefully I'm a bit more clear about this time..

Regards;
Yann.