On Tue, Oct 6, 2015 at 11:31 AM, Plüm, Rüdiger, Vodafone Group <[email protected]> wrote: > > >> -----Original Message----- >> From: Yann Ylavic [mailto:[email protected]] >> Sent: Dienstag, 6. Oktober 2015 11:13 >> To: [email protected] >> Subject: Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ >> modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ >> server/mpm/simple/ >> >> On Tue, Oct 6, 2015 at 9:37 AM, Plüm, Rüdiger, Vodafone Group >> <[email protected]> wrote: >> > >> >> -----Original Message----- >> >> From: Yann Ylavic [mailto:[email protected]] >> >> >> >> > Modified: httpd/httpd/trunk/server/mpm/event/event.c >> >> > URL: >> >> >> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?re >> >> v=1706669&r1=1706668&r2=1706669&view=diff >> >> > >> >> >> ========================================================================== >> >> ==== >> >> > --- httpd/httpd/trunk/server/mpm/event/event.c (original) >> >> > +++ httpd/httpd/trunk/server/mpm/event/event.c Sun Oct 4 10:10:51 >> 2015 >> >> > @@ -1146,19 +1146,38 @@ read_request: >> >> > } >> >> > >> >> [] >> >> > + >> >> > + rindex = apr_hash_first(NULL, c->filters); >> >> > + while (rindex) { >> >> > + ap_filter_t *f = apr_hash_this_val(rindex); >> >> > + >> >> > + if (!APR_BRIGADE_EMPTY(f->bb)) { >> >> > + >> >> > + rv = ap_pass_brigade(f, c->empty); >> >> > + apr_brigade_cleanup(c->empty); >> >> > + if (APR_SUCCESS != rv) { >> >> > + ap_log_cerror( >> >> > + APLOG_MARK, APLOG_DEBUG, rv, c, >> >> APLOGNO(00470) >> >> > + "write failure in '%s' output filter", >> f- >> >> >frec->name); >> >> > + break; >> >> > + } >> >> > + >> >> > + if (ap_filter_should_yield(f)) { >> >> > + data_in_output_filters = 1; >> >> > + } >> >> > + } >> >> > + >> >> > + rindex = apr_hash_next(rindex); >> >> > } >> >> >> >> This pattern looks shared by all (relevant) MPMs, couldn't we make it >> >> a function? Maybe ap_reinstate_conn()? >> > >> > Sounds sensible. >> >> By defining this function in util_filter >> (ap_filter_reinstate_conn()?), we also possibly could get rid of >> c->empty (which looks quite hacky), using/hidding an empty brigade per >> filter (in opaque data). > > Although the brigade is empty, don't we need to prevent the brigade being > used by multiple threads in parallel? > Using one per connection prevents this.
Hmm, the filters are also allocated per connection/request (on c/r->pool), and shouldn't be called concurrently right? BTW, I wonder if we can "reinstate" the filters in arbitrary order like in the above loop (the order seems to depend on the calls to ap_filter_setaside_brigade() and internal apr_hash_t ordering). Don't we need to start from r->ouput_filters down to the last filter and call ap_pass_brigade(f, c->empty) if f is in the hashtable?
