RE: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/

[Plüm , Rüdiger , Vodafone Group]

> -----Original Message-----
> From: Yann Ylavic [mailto:ylavic....@gmail.com]
> Sent: Dienstag, 6. Oktober 2015 11:13
> To: dev@httpd.apache.org
> 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
> <ruediger.pl...@vodafone.com> wrote:
> >
> >> -----Original Message-----
> >> From: Yann Ylavic [mailto:ylavic....@gmail.com]
> >>
> >> > 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.

Regards

Rüdiger