Yann, I changed the access after free, thanks again(!) for reviewing this.
The spare pool I left like this for now. I think, *if* there is only one spare, it may be beneficial to keep the newest one, as a) that might be hot and b) it destroys pools more frequently and may recycle memory better. But pbly not a big difference... //Stefan > Am 16.11.2015 um 17:30 schrieb Yann Ylavic <[email protected]>: > > On Mon, Nov 16, 2015 at 4:12 PM, <[email protected]> wrote: >> Author: icing >> Date: Mon Nov 16 15:12:35 2015 >> New Revision: 1714602 >> >> URL: http://svn.apache.org/viewvc?rev=1714602&view=rev >> Log: >> improvements in handling orphaned stream resources, where client prematurely >> closed it, guarantueed session shutdown, slave connections created 2.5-DEV >> style >> > [] >> Modified: httpd/httpd/trunk/modules/http2/h2_mplx.c >> URL: >> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_mplx.c?rev=1714602&r1=1714601&r2=1714602&view=diff >> ============================================================================== >> --- httpd/httpd/trunk/modules/http2/h2_mplx.c (original) >> +++ httpd/httpd/trunk/modules/http2/h2_mplx.c Mon Nov 16 15:12:35 2015 >> @@ -227,24 +227,22 @@ void h2_mplx_abort(h2_mplx *m) >> >> static void io_destroy(h2_mplx *m, h2_io *io) >> { >> - if (io) { >> - apr_pool_t *pool = io->pool; >> - if (pool) { >> - io->pool = NULL; >> - apr_pool_clear(pool); >> - if (m->spare_pool) { >> - apr_pool_destroy(m->spare_pool); >> - } >> - m->spare_pool = pool; >> - } >> - /* The pool is cleared/destroyed which also closes all >> - * allocated file handles. Give this count back to our >> - * file handle pool. */ >> - m->file_handles_allowed += io->files_handles_owned; >> - h2_io_set_remove(m->stream_ios, io); >> - h2_io_set_remove(m->ready_ios, io); >> - h2_io_destroy(io); >> - } >> + apr_pool_t *pool = io->pool; >> + if (pool) { >> + io->pool = NULL; >> + apr_pool_clear(pool); >> + if (m->spare_pool) { >> + apr_pool_destroy(m->spare_pool); >> + } >> + m->spare_pool = pool; > > One spare pool only? > > Maybe we could then: > if (m->spare_pool) { > apr_pool_destroy(pool); > } > else { > apr_pool_clear(pool); > m->spare_pool = pool; > } > ? > >> + } >> + /* The pool is cleared/destroyed which also closes all >> + * allocated file handles. Give this count back to our >> + * file handle pool. */ >> + m->file_handles_allowed += io->files_handles_owned; >> + h2_io_set_remove(m->stream_ios, io); >> + h2_io_set_remove(m->ready_ios, io); >> + h2_io_destroy(io); > > Looks like io is desallocated here (with apr_pool_clear/destroy() > above), can we still use it at that point? > >> } > > Regards, > Yann.
