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
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(_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
On Thu, Jul 1, 2021 at 12:38 PM Yann Ylavic wrote:
>
> On Wed, Jun 30, 2021 at 9:42 AM Joe Orton 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 == _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(_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.
Index: include/ap_mmn.h
===
--- include/ap_mmn.h (revision 1892424)
+++ include/ap_mmn.h (working copy)
@@ -673,7 +673,7 @@
* ap_proxy_tunnel_conn_get_transferred() change
* ap_proxy_transfer_between_connections() sent to apr_off_t *.
* 20210531.0 (2.5.1-dev) add conn_rec->outgoing and ap_ssl_bind_outgoing()
- * 20210531.1 (2.5.1-dev) Add ap_bucket_type_wc, ap_bucket_wc_make() and
+ * 20210531.1 (2.5.1-dev) Add ap_bucket_wc_data, ap_bucket_wc_make() and
* ap_bucket_wc_create() to util_filter.h
* 20210531.2 (2.5.1-dev) Add ap_proxy_get_worker_ex() and
* ap_proxy_define_worker_ex() to mod_proxy.h
Index: include/util_filter.h
===
--- include/util_filter.h (revision 1892424)
+++ include/util_filter.h (working copy)
@@ -763,15 +763,31 @@ AP_DECLARE(void) ap_filter_protocol(ap_filter_t* f
/** Filter is incompatible with "Cache-Control: no-transform" */
#define AP_FILTER_PROTO_TRANSFORM 0x20
-/** Write Completion (WC) bucket */
-AP_DECLARE_DATA extern const apr_bucket_type_t ap_bucket_type_wc;
+/**
+ * @brief Write Completion (WC) bucket
+ *
+ * A WC bucket is a FLUSH bucket with special ->data == _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).
+ */
+/** Write Completion (WC) bucket data mark */
+AP_DECLARE_DATA extern const char ap_bucket_wc_data;
+
/**
* Determine if a bucket is a Write Completion (WC) bucket
* @param e The bucket to inspect
* @return true or false
*/
-#define AP_BUCKET_IS_WC(e) ((e)->type == _bucket_type_wc)
+#define AP_BUCKET_IS_WC(e) (APR_BUCKET_IS_FLUSH(e) && \
+(e)->data == (void *)_bucket_wc_data)
/**
* Make the bucket passed in a Write Completion (WC) bucket
Index: server/util_filter.c
===
--- server/util_filter.c (revision 1892424)
+++ server/util_filter.c (working copy)
@@ -976,7 +976,9 @@ AP_DECLARE(apr_status_t) ap_filter_setaside_brigad
e = next) {
next = APR_BUCKET_NEXT(e);
-/* Strip WC buckets added by ap_filter_output_pending(). */
+/* WC buckets will be added back by ap_filter_output_pending()
+ * at the tail.
+ */
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
On Wed, Jun 30, 2021 at 9:42 AM Joe Orton wrote: > > On Tue, Jun 29, 2021 at 09:16:21PM -, yla...@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.
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
On Tue, Jun 29, 2021 at 09:16:21PM -, yla...@apache.org wrote: > Author: ylavic > Date: Tue Jun 29 21:16:21 2021 > New Revision: 1891148 > > URL: http://svn.apache.org/viewvc?rev=1891148=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
