Re: svn commit: r1885605 - in /httpd/httpd/branches/2.4.x: ./ include/ modules/proxy/
On Tue, Apr 20, 2021 at 12:40 PM Ruediger Pluem 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. Index: include/util_filter.h === --- include/util_filter.h (revision 1889852) +++ include/util_filter.h (working copy) @@ -763,7 +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; + /** + * 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 == &ap_bucket_type_wc) + +/** + * Make the bucket passed in a Write Completion (WC) bucket + * @param b The bucket to make into a WC bucket + * @return The new bucket, or NULL if allocation failed + */ +AP_DECLARE(apr_bucket *) ap_bucket_wc_make(apr_bucket *b); + +/** + * Create a bucket referring to a Write Completion (WC). + * @param list The freelist from which this bucket should be allocated + * @return The new bucket, or NULL if allocation failed + */ +AP_DECLARE(apr_bucket *) ap_bucket_wc_create(apr_bucket_alloc_t *list); + +/** * @} */ Index: server/util_filter.c === --- server/util_filter.c (revision 1889852) +++ server/util_filter.c (working copy) @@ -976,6 +976,12 @@ 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(). */ +if (AP_BUCKET_IS_WC(e)) { +apr_bucket_delete(e); +continue; +} + /* Opaque buckets (length == -1) are moved, so assumed to have * next EOR's lifetime or at least the lifetime of the connection. */ @@ -1268,7 +1274,10 @@ AP_DECLARE_NONSTD(int) ap_filter_output_pending(co if (!APR_BRIGADE_EMPTY(fp->bb)) { ap_filter_t *f = fp->f; apr_status_t rv; +apr_bucket *b; +b = ap_bucket_wc_create(bb->bucket_alloc); +APR_BRIGADE_INSERT_TAIL(bb, b); rv = ap_pass_brigade(f, bb); apr_brigade_cleanup(bb); @@ -1279,8 +1288,7 @@ AP_DECLARE_NONSTD(int) ap_filter_output_pending(co break; } -if ((fp->bb && !APR_BRIGADE_EMPTY(fp->bb)) -|| (f->next && ap_filter_should_yield(f->next))) { +if (ap_filter_should_yield(f)) { rc = OK; break; } @@ -1391,3 +1399,41 @@ AP_DECLARE(void) ap_filter_protocol(ap_filter_t *f { f->frec->proto_flags = flags ; } + + +static apr_status_t wc_bucket_read(apr_bucket *b, const ch
Re: svn commit: r1885605 - in /httpd/httpd/branches/2.4.x: ./ include/ modules/proxy/
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
Re: svn commit: r1885605 - in /httpd/httpd/branches/2.4.x: ./ include/ modules/proxy/
On Sun, Apr 18, 2021 at 7:37 PM Christophe JAILLET 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 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.
Re: svn commit: r1885605 - in /httpd/httpd/branches/2.4.x: ./ include/ modules/proxy/
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
Re: svn commit: r1885605 - in /httpd/httpd/branches/2.4.x: ./ include/ modules/proxy/
> 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.
Re: svn commit: r1885605 - in /httpd/httpd/branches/2.4.x: ./ include/ modules/proxy/
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 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
Re: svn commit: r1885605 - in /httpd/httpd/branches/2.4.x: ./ include/ modules/proxy/
Le 17/01/2021 à 17:21, minf...@apache.org a écrit : Author: minfrin Date: Sun Jan 17 16:21:35 2021 New Revision: 1885605 URL: http://svn.apache.org/viewvc?rev=1885605&view=rev Log: Backport to v2.4: *) mod_proxy_http: handle upgrade/tunneling protocols. BZ 61616 is about mod_proxy_connect but there has been wstunnel reports on dev@ about that too lately. trunk patch: https://svn.apache.org/r1678771 https://svn.apache.org/r1832348 https://svn.apache.org/r1869338 https://svn.apache.org/r1869420 https://svn.apache.org/r1878367 https://svn.apache.org/r1877557 https://svn.apache.org/r1877558 Here https://svn.apache.org/r1877646 https://svn.apache.org/r1877695 https://svn.apache.org/r1879401 https://svn.apache.org/r1879402 https://svn.apache.org/r1880200 https://svn.apache.org/r1885239 https://svn.apache.org/r1885240 https://svn.apache.org/r1885244 2.4.x patch: http://people.apache.org/~ylavic/patches/2.4.x-mod_proxy_http-upgrade-4on5-v2.patch https://github.com/apache/httpd/pull/158 +1: ylavic, covener, minfrin ylavic: All the corresponding trunk changes to mod_proxy_wstunnel (but r1885239) have been dropped for this backport proposal, the goal being to handle upgrade in mod_proxy_http from now, while r1885239 allows to benefit from the Upgrade improvements done in proxy_http with existing wstunnel configurations (provided mod_proxy_http module is loaded). Modified: httpd/httpd/branches/2.4.x/CHANGES httpd/httpd/branches/2.4.x/STATUS httpd/httpd/branches/2.4.x/include/ap_mmn.h httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.h httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy_connect.c httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy_http.c httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy_wstunnel.c httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c [...] 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? CJ
Re: svn commit: r1885605 - in /httpd/httpd/branches/2.4.x: ./ include/ modules/proxy/
On 2/2/21 4:18 PM, Yann Ylavic wrote: > On Tue, Feb 2, 2021 at 10:32 AM Ruediger Pluem 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
Re: svn commit: r1885605 - in /httpd/httpd/branches/2.4.x: ./ include/ modules/proxy/
On Tue, Feb 2, 2021 at 10:32 AM Ruediger Pluem 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.
Re: svn commit: r1885605 - in /httpd/httpd/branches/2.4.x: ./ include/ modules/proxy/
On 1/17/21 5:21 PM, minf...@apache.org wrote: > Author: minfrin > Date: Sun Jan 17 16:21:35 2021 > New Revision: 1885605 > > URL: http://svn.apache.org/viewvc?rev=1885605&view=rev > Log: > Backport to v2.4: > > *) mod_proxy_http: handle upgrade/tunneling protocols. BZ 61616 is about > mod_proxy_connect but there has been wstunnel reports > on dev@ about that too lately. > trunk patch: https://svn.apache.org/r1678771 > https://svn.apache.org/r1832348 > https://svn.apache.org/r1869338 > https://svn.apache.org/r1869420 > https://svn.apache.org/r1878367 > https://svn.apache.org/r1877557 > https://svn.apache.org/r1877558 > https://svn.apache.org/r1877646 > https://svn.apache.org/r1877695 > https://svn.apache.org/r1879401 > https://svn.apache.org/r1879402 > https://svn.apache.org/r1880200 > https://svn.apache.org/r1885239 > https://svn.apache.org/r1885240 > https://svn.apache.org/r1885244 > 2.4.x patch: > http://people.apache.org/~ylavic/patches/2.4.x-mod_proxy_http-upgrade-4on5-v2.patch > https://github.com/apache/httpd/pull/158 > +1: ylavic, covener, minfrin > ylavic: All the corresponding trunk changes to mod_proxy_wstunnel (but > r1885239) have been dropped for this backport proposal, the goal > being to handle upgrade in mod_proxy_http from now, while > r1885239 > allows to benefit from the Upgrade improvements done in > proxy_http > with existing wstunnel configurations (provided mod_proxy_http > module is loaded). > > > Modified: > httpd/httpd/branches/2.4.x/CHANGES > httpd/httpd/branches/2.4.x/STATUS > httpd/httpd/branches/2.4.x/include/ap_mmn.h > httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c > httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.h > httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy_connect.c > httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy_http.c > httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy_wstunnel.c > httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c > > Modified: httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c > URL: > http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c?rev=1885605&r1=1885604&r2=1885605&view=diff > == > --- httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c (original) > +++ httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c Sun Jan 17 16:21:35 > 2021 i, > @@ -4119,81 +4168,498 @@ PROXY_DECLARE(apr_status_t) ap_proxy_tra > const char *name, > int *sent, > apr_off_t bsize, > - int after) > + int flags) > { > apr_status_t rv; > +int flush_each = 0; > +unsigned int num_reads = 0; > #ifdef DEBUGGING > apr_off_t len; > #endif > > -do { > +/* > + * Compat: since FLUSH_EACH is default (and zero) for legacy reasons, we > + * pretend it's no FLUSH_AFTER nor YIELD_PENDING flags, the latter > because > + * flushing would defeat the purpose of checking for pending data (hence > + * determine whether or not the output chain/stack is full for stopping). > + */ > +if (!(flags & (AP_PROXY_TRANSFER_FLUSH_AFTER | > + AP_PROXY_TRANSFER_YIELD_PENDING))) { > +flush_each = 1; > +} > + > +for (;;) { > apr_brigade_cleanup(bb_i); > rv = ap_get_brigade(c_i->input_filters, bb_i, AP_MODE_READBYTES, > APR_NONBLOCK_READ, bsize); > -if (rv == APR_SUCCESS) { > -if (c_o->aborted) { > -return APR_EPIPE; > -} > -if (APR_BRIGADE_EMPTY(bb_i)) { > -break; > +if (rv != APR_SUCCESS) { > +if (!APR_STATUS_IS_EAGAIN(rv) && !APR_STATUS_IS_EOF(rv)) { > +ap_log_rerror(APLOG_MARK, APLOG_DEBUG, rv, r, APLOGNO(03308) > + "ap_proxy_transfer_between_connections: " > + "error on %s - ap_get_brigade", > + name); > +if (rv == APR_INCOMPLETE) { > +/* Don't return APR_INCOMPLETE, it'd mean "should yield" > + * for the caller, while it means "incomplete body" here > + * from ap_http_filter(), which is an error. > + */ > +rv = APR_E