Is mpm_event missing allocator mutexes?

[Stefan Fritsch <sf...@sfritsch.de>]

Hi,

I may be missing something but this looks wrong to me:

apr_allocator uses a mutex to be thread safe. Pools use this mutex also to 
protect sub-pool creation, cleanup registering, etc. When apr creates the 
initial allocator and global_pool in apr_pool_initialize(), it also creates a 
mutex for this allocator. But mpm_event creates a new allocator for each 
transaction pool and does not create a mutex for it. And gdb shows that in 
listener_thread() at the get_worker() call, ptrans->allocator->mutex is NULL 
(checked in 2.4.25).

Shouldn't we allocate a mutex there and call apr_allocator_mutex_set()? If no, 
why not. If yes, why does it still work as well as it does right now? Or could 
the lacking mutex explain some weird segfaults?

Cheers,
Stefan

Re: Is mpm_event missing allocator mutexes?

[Yann Ylavic <yl...@gmail.com>]

Hi Stefan,

On Sun, Feb 5, 2017 at 10:39 PM, Stefan Fritsch <sf...@sfritsch.de> wrote:
>
> apr_allocator uses a mutex to be thread safe. Pools use this mutex also to
> protect sub-pool creation, cleanup registering, etc. When apr creates the
> initial allocator and global_pool in apr_pool_initialize(), it also creates a
> mutex for this allocator. But mpm_event creates a new allocator for each
> transaction pool and does not create a mutex for it. And gdb shows that in
> listener_thread() at the get_worker() call, ptrans->allocator->mutex is NULL
> (checked in 2.4.25).

Howesome catch!

>
> Shouldn't we allocate a mutex there and call apr_allocator_mutex_set()? If no,
> why not. If yes, why does it still work as well as it does right now? Or could
> the lacking mutex explain some weird segfaults?

I think it always worked with HTTP/1 because we never race on request
create/destroy (i.e. on creation/destruction of ptrans' subpools) nor,
AFAICT, with subpools of the request (usually used by the same
thread).
We do have concurrent threads with subpools but they are created from
pconf/pchild (hence mutexed).

With HTTP/2 things get complicated because there are multiple subpools
(of subpools) of ptrans (slave connections, mplx, ...) concurrently
created/destroyed, and that obviously can't work without a mutex.

Maybe since 2.4.25, and the changes wrt kept alive connections early
closing on graceful (or/and the mpm event wakeup patch, not sure
whether s.priebe@ finally runs mpm_event w/ or w/o it applied), we get
more concurrency with ptrans being destroyed "out of band" (while h2
still has ongoing work).

Anyway, it also seems to me that we should make ptrans mutexed, and so
do we for all allocators dedicated to pools created in mod_http2.


Regards,
Yann.