> -----Original Message----- > From: Yann Ylavic [mailto:[email protected]] > 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 <[email protected]> wrote: > > On 11 Sep 2014, at 1:51 AM, Yann Ylavic <[email protected]> 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
