Re: svn commit: r1891148 - in /httpd/httpd/trunk: include/ap_mmn.h include/util_filter.h modules/proxy/proxy_util.c server/util_filter.c

[Joe Orton <jo...@redhat.com>]

On Tue, Jun 29, 2021 at 09:16:21PM -0000, ylavic@apache.org wrote:
> Author: ylavic
> Date: Tue Jun 29 21:16:21 2021
> New Revision: 1891148
> 
> URL: http://svn.apache.org/viewvc?rev=1891148&view=rev
> Log:
> core: Write Completion (WC) bucket type.
> 
> A WC bucket is meant to prevent buffering/coalescing filters from retaining
> data, but unlike a FLUSH bucket it won't cause the core output filter to
> block trying to flush anything before.

Interesting.  I think it would be helpful to have the semantics of this 
bucket type described in the header as well.

So filters should treat a WC bucket the same as FLUSH in general?  And 
specifically, filters are broken if they don't?  It seems like an 
accident that this makes mod_ssl's coalesce filter flush, merely because 
it will flush for *any* metadata bucket type, but it didn't have to be 
designed that way.

If so I wonder if it wouldn't be better to overload FLUSH for this, e.g. 
by having a FLUSH bucket with a non-NULL ->data pointer or something? 
The core knows it is special but everywhere else treats as FLUSH.

(Seems we need bucket subclasses...)

Regards, Joe

Re: svn commit: r1891148 - in /httpd/httpd/trunk: include/ap_mmn.h include/util_filter.h modules/proxy/proxy_util.c server/util_filter.c

[Joe Orton <jo...@redhat.com>]

On Thu, Aug 19, 2021 at 03:28:44PM +0200, Yann Ylavic wrote:
> The only filters that care about write completion for now are
> ap_core_output_filter() and ssl_io_filter_output(), which try to fill
> in the pipe as much as possible, using
> ap_filter_reinstate_brigade(&flush_upto) to determine whether they
> should flush (blocking) or setaside their remaining data.
> 
> So ap_filter_reinstate_brigade() is made to not treat WC as FLUSH
> buckets and keep the above filters working as before (and correctly
> w.r.t WC bucket semantics). I first thought adding a new
> ap_filter_rec->proto_flags like AP_FILTER_PROTO_WC_READY to do this
> only for filter registered with this flag, and then register
> ap_core_output_filter() and ssl_io_filter_output() accordingly, but
> since they are the only users of ap_filter_reinstate_brigade() with
> flush_upto != NULL I kept it simple for now..
> 
> WDYT?

+1 

Very nice, I like it, thanks Yann.

Regards, Joe

Re: svn commit: r1891148 - in /httpd/httpd/trunk: include/ap_mmn.h include/util_filter.h modules/proxy/proxy_util.c server/util_filter.c

[Yann Ylavic <yl...@gmail.com>]

On Thu, Jul 1, 2021 at 12:38 PM Yann Ylavic <yl...@gmail.com> wrote:
>
> On Wed, Jun 30, 2021 at 9:42 AM Joe Orton <jo...@redhat.com> wrote:
> >
> > If so I wonder if it wouldn't be better to overload FLUSH for this, e.g.
> > by having a FLUSH bucket with a non-NULL ->data pointer or something?
> > The core knows it is special but everywhere else treats as FLUSH.
>
> That's a great idea, let me try that.

Here is a new patch, the semantics of WC buckets are defined as:
/**
 * @brief Write Completion (WC) bucket
 *
 * A WC bucket is a FLUSH bucket with special ->data == &ap_bucket_wc_data,
 * still both AP_BUCKET_IS_WC() and APR_BUCKET_IS_FLUSH() hold for them so
 * they have the same semantics for most filters, namely:
 *   Everything produced before shall be passed to the next filter, including
 *   the WC/FLUSH bucket itself.
 * The distinction between WC and FLUSH buckets is only for filters that care
 * about write completion (calling ap_filter_reinstate_brigade() with non-NULL
 * flush_upto), those can setaside WC buckets and the preceding data provided
 * they have first determined that the next filter(s) have pending data
 * already, usually by calling ap_filter_should_yield(f->next).
 */

The only filters that care about write completion for now are
ap_core_output_filter() and ssl_io_filter_output(), which try to fill
in the pipe as much as possible, using
ap_filter_reinstate_brigade(&flush_upto) to determine whether they
should flush (blocking) or setaside their remaining data.

So ap_filter_reinstate_brigade() is made to not treat WC as FLUSH
buckets and keep the above filters working as before (and correctly
w.r.t WC bucket semantics). I first thought adding a new
ap_filter_rec->proto_flags like AP_FILTER_PROTO_WC_READY to do this
only for filter registered with this flag, and then register
ap_core_output_filter() and ssl_io_filter_output() accordingly, but
since they are the only users of ap_filter_reinstate_brigade() with
flush_upto != NULL I kept it simple for now..

WDYT?

Cheers;
Yann.

Re: svn commit: r1891148 - in /httpd/httpd/trunk: include/ap_mmn.h include/util_filter.h modules/proxy/proxy_util.c server/util_filter.c

[Yann Ylavic <yl...@gmail.com>]

On Wed, Jun 30, 2021 at 9:42 AM Joe Orton <jo...@redhat.com> wrote:
>
> On Tue, Jun 29, 2021 at 09:16:21PM -0000, ylavic@apache.org wrote:
> >
> > A WC bucket is meant to prevent buffering/coalescing filters from retaining
> > data, but unlike a FLUSH bucket it won't cause the core output filter to
> > block trying to flush anything before.
>
> Interesting.  I think it would be helpful to have the semantics of this
> bucket type described in the header as well.

If I'm not mistaken, a simple way to describe FLUSH semantics is:
FLUSH buckets can't be retained/setaside by a filter.

For WC buckets, it would be: WC buckets can't be retained/setaside by
a filter UNLESS the next filters still have pending data after passing
them a trailing WC bucket.

I think this means for most filters that yes, they are the same.

A better idea for the description of the semantics in ap_filter.h?

>
> So filters should treat a WC bucket the same as FLUSH in general?  And
> specifically, filters are broken if they don't?

Yes (almost then), and yes because WC buckets would break pollers (MPM
event, proxy_tunnel) that wait for write completion.


>  It seems like an
> accident that this makes mod_ssl's coalesce filter flush, merely because
> it will flush for *any* metadata bucket type, but it didn't have to be
> designed that way.

Yeah, that's why I started simple with a new META bucket type, that
was enough for the SSL coalesce case.
I think it's useful for async in general where we want to never block,
on the ap_core_output_filter() side we possibly want to disable
FlushMaxThreshold when a WC is found, for instance.
As for the SSL coalesce filter, it possibly could be a core filter on
its own (at AP_FTYPE_CONNECTION + 4 still) so that we could apply a
FlushMinThreshold for both TLS and plain connections.

>
> If so I wonder if it wouldn't be better to overload FLUSH for this, e.g.
> by having a FLUSH bucket with a non-NULL ->data pointer or something?
> The core knows it is special but everywhere else treats as FLUSH.

That's a great idea, let me try that.

>
> (Seems we need bucket subclasses...)

Oh, I thought we had that already, with (poly)morphism and so on :)


Thanks;
Yann.