httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Plüm, Rüdiger, Vodafone Group <ruediger.pl...@vodafone.com>
Subject RE: [Patch] Async write completion for the full connection filter stack
Date Thu, 11 Sep 2014 08:20:56 GMT


> -----Original Message-----
> From: Yann Ylavic [mailto:ylavic.dev@gmail.com]
> Sent: Donnerstag, 11. September 2014 09:52
> To: httpd
> Subject: Re: [Patch] Async write completion for the full connection filter
> stack
> 
> On Thu, Sep 11, 2014 at 6:39 AM, Graham Leggett <minfrin@sharp.fm> wrote:
> > On 11 Sep 2014, at 1:51 AM, Yann Ylavic <ylavic.dev@gmail.com> wrote:
> >
> >> +    else if (*deferred_write_pool) {
> >> +        /*
> >> +         * There are no more requests in the pipeline. We can just
> clear the
> >> +         * pool.
> >> +         */
> >>
> >> Shouldn't *buffered_bb be set to NULL here when *deferred_write_pool
> >> == (*buffered_bb)->p, or more generally
> >> apr_pool_is_ancestor(*deferred_write_pool, (*buffered_bb)->p). We
> >> can't leave a dangling pointer in this case.
> >>
> >> +        apr_pool_clear(*deferred_write_pool);
> >
> > Hmmm… this came from the original code.
> >
> >
> > We can’t set buffered_bb to NULL unless we are sure we created
> buffered_bb, and this isn’t necessarily the case. In the core filter,
> buffered_bb is created when the connection is created.

Another question is if we should clear a pool we haven't created.
If we don't then apr_pool_is_ancestor(*deferred_write_pool, (*buffered_bb)->p)
should be a save bet that we created the bucket brigade.
Another approach below.

> 
> If we or any filter has created the brigade on *deferred_write_pool
> (or an ancestor, I'm only talking about this case), it is dead now
> (unaccessible), and IMO should be marked as so.
> In the case the caller let the function handle the brigade (as we do),
> using NULL the first time only, (s)he expects to not crash whenever
> (s)he come back with it after a clear (which (s)he isn't supposed to
> know about).

Agree. Maybe if buffered_bb is NULL we should add a pool clean up to *deferred_write_pool
that sets *buffered_bb to NULL

> 
> Another solution would be to not require a pool at all, something like :

I fear that this creates high memory consumption / temporary memory leaks.

Regards

Rüdiger

Mime
View raw message