The below got me thinking... Right now, the pool allocator mutex is only used when, well, allocator_alloc() is called, which means that sometimes apr_palloc(), for example, can be thread-safeish and sometimes not, depending on whether or not the active node has enough space.
For 1.6 and later, it might be nice to actually protect the adjustment of the active node, et.al. to, if a mutex is present, always be thread-safe... that is, always when we "alloc" memory, even when/if we do/don't called allocator_alloc(). Thoughts? > On Feb 20, 2017, at 8:38 AM, yla...@apache.org wrote: > > Author: ylavic > Date: Mon Feb 20 13:38:03 2017 > New Revision: 1783755 > > URL: http://svn.apache.org/viewvc?rev=1783755&view=rev > Log: > mpm_event: use a mutex for ptrans' allocator to be safe with concurrent > creation and destruction of its subpools, like with mod_http2. > > > Modified: > httpd/httpd/trunk/server/mpm/event/event.c > > Modified: httpd/httpd/trunk/server/mpm/event/event.c > URL: > http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1783755&r1=1783754&r2=1783755&view=diff > ============================================================================== > --- httpd/httpd/trunk/server/mpm/event/event.c (original) > +++ httpd/httpd/trunk/server/mpm/event/event.c Mon Feb 20 13:38:03 2017 > @@ -2096,6 +2096,7 @@ static void * APR_THREAD_FUNC listener_t > } > if (!listeners_disabled) { > void *csd = NULL; > + apr_thread_mutex_t *mutex; > ap_listen_rec *lr = (ap_listen_rec *) pt->baton; > apr_pool_t *ptrans; /* Pool for per-transaction > stuff */ > ap_pop_pool(&ptrans, worker_queue_info); > @@ -2105,20 +2106,44 @@ static void * APR_THREAD_FUNC listener_t > apr_allocator_t *allocator; > > apr_allocator_create(&allocator); > - apr_allocator_max_free_set(allocator, > - ap_max_mem_free); > + apr_allocator_max_free_set(allocator, > ap_max_mem_free); > apr_pool_create_ex(&ptrans, pconf, NULL, allocator); > - apr_allocator_owner_set(allocator, ptrans); > if (ptrans == NULL) { > ap_log_error(APLOG_MARK, APLOG_CRIT, rc, > ap_server_conf, APLOGNO(03097) > "Failed to create transaction pool"); > + apr_allocator_destroy(allocator); > signal_threads(ST_GRACEFUL); > return NULL; > } > + apr_allocator_owner_set(allocator, ptrans); > } > apr_pool_tag(ptrans, "transaction"); > > + /* We need a mutex in the allocator to synchronize > ptrans' > + * children creations/destructions, but this mutex ought > to > + * live in ptrans itself to avoid leaks, hence it's > cleared > + * in ap_push_pool(). We could recycle some pconf's > mutexes > + * like we do for ptrans subpools, but that'd need > another > + * synchronization mechanism, whereas creating a pthread > + * mutex (unix here!) is really as simple/fast as a > static > + * PTHREAD_MUTEX_INIT assignment, so let's not bother and > + * create the mutex for each ptrans (recycled or not). > + */ > + rc = apr_thread_mutex_create(&mutex, > + APR_THREAD_MUTEX_DEFAULT, > + ptrans); > + if (rc != APR_SUCCESS) { > + ap_log_error(APLOG_MARK, APLOG_CRIT, rc, > + ap_server_conf, APLOGNO() > + "Failed to create transaction pool > mutex"); > + ap_push_pool(worker_queue_info, ptrans); > + signal_threads(ST_GRACEFUL); > + return NULL; > + } > + apr_allocator_mutex_set(apr_pool_allocator_get(ptrans), > + mutex); > + > get_worker(&have_idle_worker, 1, &workers_were_busy); > rc = lr->accept_func(&csd, lr, ptrans); > > >
