> -----Original Message----- > From: Yann Ylavic [mailto:[email protected]] > Sent: Dienstag, 6. Oktober 2015 01:40 > 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/ > > > 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. > > Also it seems that we leak the hash iterator here (on c->pool). Why do we leak here? apr_hash_first(NULL, is used. So no new iterator is allocated. > Couldn't we use a (single) linked list for c->filters since > ap_filter_t already has a 'next' field? > > > > > Modified: httpd/httpd/trunk/server/util_filter.c > > URL: > http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util_filter.c?rev=17 > 06669&r1=1706668&r2=1706669&view=diff > > > ========================================================================== > ==== > > --- httpd/httpd/trunk/server/util_filter.c (original) > > +++ httpd/httpd/trunk/server/util_filter.c Sun Oct 4 10:10:51 2015 > [] > > @@ -635,7 +653,8 @@ AP_DECLARE(apr_status_t) ap_save_brigade > > apr_status_t rv, srv = APR_SUCCESS; > > > > /* If have never stored any data in the filter, then we had better > > - * create an empty bucket brigade so that we can concat. > > + * create an empty bucket brigade so that we can concat. Register > > + * a cleanup to zero out the pointer if the pool is cleared. > > Maybe this comment belongs in ap_filter_setaside_brigade()? > > > */ > > if (!(*saveto)) { > > *saveto = apr_brigade_create(p, f->c->bucket_alloc); > > @@ -673,6 +692,248 @@ AP_DECLARE(apr_status_t) ap_save_brigade > > return srv; > > } > > > > +static apr_status_t filters_cleanup(void *data) > > +{ > > + ap_filter_t **key = data; > > + > > + apr_hash_set((*key)->c->filters, key, sizeof(ap_filter_t **), > NULL); > > + > > + return APR_SUCCESS; > > +} > > Shouldn't we call filters_cleanup() from ap_remove_output_filter() too? I think so. > > + > > + f->bb = apr_brigade_create(pool, f->c->bucket_alloc); > > + > > + apr_pool_pre_cleanup_register(pool, key, filters_cleanup); > > + > > + } > > + > > + /* decide what pool we setaside to, request pool or deferred > pool? */ > > + if (f->r) { > > + pool = f->r->pool; > > + APR_BRIGADE_CONCAT(f->bb, bb); > > + } > > + else { > > + if (!f->deferred_pool) { > > + apr_pool_create(&f->deferred_pool, f->c->pool); > > + apr_pool_tag(f->deferred_pool, "deferred_pool"); > > + } > > + pool = f->deferred_pool; > > + return ap_save_brigade(f, &f->bb, &bb, pool); > > + } > > Shouldn't ap_save_brigade() be moved below the "else"? Looks like it should, plus removing APR_BRIGADE_CONCAT(f->bb, bb); above. I think we need to set aside the buckets of bb even if we have f->r as bb may contain transient buckets. Regards Rüdiger
