Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
On Tue, Feb 21, 2017 at 3:12 PM, Stefan Eissing wrote: > > I think at this stage, let's get all changes/reverts into trunk. When that is > all done, we can talk about changes that I "expect" to work additionally. Reverted now (but the allocator mutex in h2_slave_create(), up to you ;)
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
> Am 21.02.2017 um 14:54 schrieb Yann Ylavic : > > We seem to be talking past each other, I'll _try_ to synchronize ourselves :) We'll get there! :) > On Tue, Feb 21, 2017 at 1:57 PM, Stefan Eissing wrote: >> >> You have me confused now. If you did that all only for h2, then, I >> think, we can live without them all nowadays, because: > > Actually I made the MPMs change mostly for correctness with regard to > any possible module. > > But since it is useless for http/1 and may hurt common performances, I think > I'll revert the changes on the MPMs sides (provided mplx->pool is kept > mutexed!). That is totally fine with me. Please revert that and I make the necessary changes to h2 and run my stress tests, make a v1.9.1 and let others verify this. >> 1. master conn_rec->pool (own allocator, all ops in one thread) >> * h2_session->pool >>- h2_stream->pool >>- h2_mplx->pool >> >> 2. h2_mplx->pool (own allocator, all ops guarded h2_mplx->lock mutex): >> * h2_slave->pool > > OK, so for the crashes you reported yesterday, both MPM's ptrans (aka > master->pool) and mplx->pool were without a mutex. This is what you > tested right? > If so, this is "expected" to crash (at least understood now, sf > pointed us to this in another thread), because nothing is protecting > concurrent creation/destruction of mplx->pool's subpools. I think at this stage, let's get all changes/reverts into trunk. When that is all done, we can talk about changes that I "expect" to work additionally. > [...] >> >> This is how it is intended to be used. > > Got it (I think :) :) > It should be both safe an performant, could you pass your stress tests on it? > That is, with current trunk, comment out the apr_allocator_mutex_set() > used in mpm_event's listener_thread() and h2_slave_create(). > > Unless I (again) misunderstood what you tried to tell me :) Will do, once you revert. > > Thanks! > Yann. Stefan Eissing bytes GmbH Hafenstrasse 16 48155 Münster www.greenbytes.de
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
We seem to be talking past each other, I'll _try_ to synchronize ourselves :) On Tue, Feb 21, 2017 at 1:57 PM, Stefan Eissing wrote: > > You have me confused now. If you did that all only for h2, then, I > think, we can live without them all nowadays, because: Actually I made the MPMs change mostly for correctness with regard to any possible module. But since it is useless for http/1 and may hurt common performances, I think I'll revert the changes on the MPMs sides (provided mplx->pool is kept mutexed!). > > 1. master conn_rec->pool (own allocator, all ops in one thread) > * h2_session->pool > - h2_stream->pool > - h2_mplx->pool > > 2. h2_mplx->pool (own allocator, all ops guarded h2_mplx->lock mutex): > * h2_slave->pool OK, so for the crashes you reported yesterday, both MPM's ptrans (aka master->pool) and mplx->pool were without a mutex. This is what you tested right? If so, this is "expected" to crash (at least understood now, sf pointed us to this in another thread), because nothing is protecting concurrent creation/destruction of mplx->pool's subpools. If either ptrans or mplx->pool (or both) is assigned a mutex, nothing should crash (the allocator is inherited). And if mplx->pool is mutexed, ptrans doesn't need to because mplxs are created in the usual MPM worker path (where ptrans is dedicated): run_process_connection => h2_h2_process_conn => h2_conn_setup => h2_session_create => h2_mplx_create (or the Upgrade path which isn't an issue either). > > 3. h2_slave->pool (own allocator, all ops in one thread) > * h2_task->pool > * r->pool OK, I missed that slave => task/request were single threaded, so slave->pool doesn't need a mutex either. It can still have its own allocator to remove useless contention on mplx->pool's mutex, but no mutex needed for itself or its children. > > This is how it is intended to be used. Got it (I think :) > And 2+3 run without allocator > mutex at Stefan Priebe's sites. Not sure, Stefan Priebe runs with many patches AIUI, including mpm_event ptrans' mutex, mplx->pool's mutex (and also useless slave->pool's), so he seems to be bullet proof. To conclude, I think all we need is an unmutexed allocator for MPMs' ptrans (i.e. revert yesterday's related changes), a mutexed allocator for mplx->pool, and an unmutexed allocator for slave->pool. It should be both safe an performant, could you pass your stress tests on it? That is, with current trunk, comment out the apr_allocator_mutex_set() used in mpm_event's listener_thread() and h2_slave_create(). Unless I (again) misunderstood what you tried to tell me :) Thanks! Yann.
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
> Am 21.02.2017 um 13:42 schrieb Yann Ylavic : > > On Mon, Feb 20, 2017 at 6:29 PM, Yann Ylavic wrote: >> On Mon, Feb 20, 2017 at 6:21 PM, Stefan Eissing >> wrote: >>> >>> 1+2, so the only allocator is from master conn_rec. >> >> The allocator with a mutex (after r1783755)? > > Sorry to insist Stefan, I see a reason why it would fail without > r1783755 (this commit) but not with, so maybe there is wolf in there > (french expression, dunno if it translates literally to english :) You have me confused now. If you did that all only for h2, then, I think, we can live without them all nowadays, because: 1. master conn_rec->pool (own allocator, all ops in one thread) * h2_session->pool - h2_stream->pool - h2_mplx->pool 2. h2_mplx->pool (own allocator, all ops guarded h2_mplx->lock mutex): * h2_slave->pool 3. h2_slave->pool (own allocator, all ops in one thread) * h2_task->pool * r->pool This is how it is intended to be used. And 2+3 run without allocator mutex at Stefan Priebe's sites. Stefan Eissing bytes GmbH Hafenstrasse 16 48155 Münster www.greenbytes.de
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
On Mon, Feb 20, 2017 at 6:29 PM, Yann Ylavic wrote: > On Mon, Feb 20, 2017 at 6:21 PM, Stefan Eissing > wrote: >> >> 1+2, so the only allocator is from master conn_rec. > > The allocator with a mutex (after r1783755)? Sorry to insist Stefan, I see a reason why it would fail without r1783755 (this commit) but not with, so maybe there is wolf in there (french expression, dunno if it translates literally to english :)
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
On Tue, Feb 21, 2017 at 12:47 PM, Jim Jagielski wrote: > >> On Feb 20, 2017, at 3:54 PM, Ruediger Pluem wrote: >> >> >> >> On 02/20/2017 02:38 PM, [email protected] 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. >> >> Hm, doesn't that impact performance too much in the HTTP/1.1 case? It >> requires the mutex to be hold not only during >> creation / destruction of subpools, but also for all apr_palloc calls that >> require the allocator to provide memory to >> the pool. I guess there will be no contention in these cases as only one >> thread will be using the pool, but still the >> lock needs to be made. >> > > I have to agree... But I'm not sure if this is an httpd issue > or an APR one related to not really thinking through allocator mutexes > and use cases. As I said realier, another possibility would be to set the mutex in mod_h2's process_connection hook (h2_h2_process_conn), such that it is scoped to http2. E.g. patched attached (+ revert of r1783755 and follow up for other MPMs). On the other hand, maybe it already works in http2 without r1783755 and friends since the mplx also creates its own mutexed allocator, and it seems that each h2 pool is a (grand-)child of mplx->pool, Stefan? Index: modules/http2/h2_h2.c === --- modules/http2/h2_h2.c (revision 1783847) +++ modules/http2/h2_h2.c (working copy) @@ -612,10 +612,24 @@ int h2_h2_process_conn(conn_rec* c) * Otherwise connection is in a fully acceptable state. * -> peek at the first 24 incoming bytes */ +apr_thread_mutex_t *mutex; apr_bucket_brigade *temp; char *s = NULL; apr_size_t slen; - + +/* We need a mutex in the allocator of the connection pool to + * synchronize subpools creations and destructions. + */ +status = apr_thread_mutex_create(&mutex, + APR_THREAD_MUTEX_DEFAULT, + c->pool); +if (status != APR_SUCCESS) { +ap_log_cerror(APLOG_MARK, APLOG_ERR, status, c, APLOGNO() + "h2_h2, error creating connection pool mutex"); +return HTTP_INTERNAL_SERVER_ERROR; +} +apr_allocator_mutex_set(apr_pool_allocator_get(c->pool), mutex); + temp = apr_brigade_create(c->pool, c->bucket_alloc); status = ap_get_brigade(c->input_filters, temp, AP_MODE_SPECULATIVE, APR_BLOCK_READ, 24);
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
> On Feb 20, 2017, at 3:54 PM, Ruediger Pluem wrote: > > > > On 02/20/2017 02:38 PM, [email protected] 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. > > Hm, doesn't that impact performance too much in the HTTP/1.1 case? It > requires the mutex to be hold not only during > creation / destruction of subpools, but also for all apr_palloc calls that > require the allocator to provide memory to > the pool. I guess there will be no contention in these cases as only one > thread will be using the pool, but still the > lock needs to be made. > I have to agree... But I'm not sure if this is an httpd issue or an APR one related to not really thinking through allocator mutexes and use cases.
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
On Mon, Feb 20, 2017 at 10:48 PM, Yann Ylavic wrote:
>
> The hard thing is that at the time ptrans is created, we don't know
> how it will be used.
> Maybe we could differ apr_allocator_mutex_set() at a later time, e.g.
> {pre,process}_connection hook?
Theorically though, a module that creates multiple threads and ptrans'
subpools to handle a connection shouldn't necessarily have to deal
with this, that's mpm_event which first created/changed the allocator
to something non thread-safe...
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
On Mon, Feb 20, 2017 at 9:54 PM, Ruediger Pluem wrote: > > > On 02/20/2017 02:38 PM, [email protected] 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. > > Hm, doesn't that impact performance too much in the HTTP/1.1 case? It > requires the mutex to be hold not only during > creation / destruction of subpools, but also for all apr_palloc calls that > require the allocator to provide memory to > the pool. I guess there will be no contention in these cases as only one > thread will be using the pool, but still the > lock needs to be made. Yes, quite possibly, I didn't try to measure it actually. The hard thing is that at the time ptrans is created, we don't know how it will be used. Maybe we could differ apr_allocator_mutex_set() at a later time, e.g. {pre,process}_connection hook? Regards, Yann.
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
On 02/20/2017 02:38 PM, [email protected] 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. Hm, doesn't that impact performance too much in the HTTP/1.1 case? It requires the mutex to be hold not only during creation / destruction of subpools, but also for all apr_palloc calls that require the allocator to provide memory to the pool. I guess there will be no contention in these cases as only one thread will be using the pool, but still the lock needs to be made. Regards Rüdiger
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
On Mon, Feb 20, 2017 at 6:21 PM, Stefan Eissing wrote: > > 1+2, so the only allocator is from master conn_rec. The allocator with a mutex (after r1783755)?
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
> Am 20.02.2017 um 17:41 schrieb Yann Ylavic : > > [Keeping httpd list here only] > > On Mon, Feb 20, 2017 at 3:51 PM, Stefan Eissing > wrote: >> >> So, apr_p*alloc() calls would be thread-safe if a mutex is set in >> the underlying allocator? Hmm, at what cost? would be my question. > > I also fear that it would be costly when not needed (e.g. request's pool). > Because yes, the allocator is inherited from the parent pool if none > is specified at creation time, and hence with this commit every child > pool of ptrans will have a mutexed allocator. > >> >> In regard to thread safety of apr_allocator, there is still something >> I do not understand. Observations: >> >> 1. When I remove the own allocator in h2_mplx, so it inherits the >> now protected one from the master connection, all runs fine. > > I'm not sure this dedicated allocator is needed, mplx seems to race > only with itself on ptrans (master->pool), but I may miss something. > >> 2. When I remove the own allocator from the slave connections also, >> in h2_conn, so that slave conns also use the protected allocator >> from the master conn, > > Hmm, rather the allocator from mplx->pool (since slave connections are > children of mplx), right? > >> things break. E.g. strange behaviour up >> to crashes under load. > > Is it with or without this commit? > Also with 1. + 2., or 2. alone? 1+2, so the only allocator is from master conn_rec.
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
[Keeping httpd list here only] On Mon, Feb 20, 2017 at 3:51 PM, Stefan Eissing wrote: > > So, apr_p*alloc() calls would be thread-safe if a mutex is set in > the underlying allocator? Hmm, at what cost? would be my question. I also fear that it would be costly when not needed (e.g. request's pool). Because yes, the allocator is inherited from the parent pool if none is specified at creation time, and hence with this commit every child pool of ptrans will have a mutexed allocator. > > In regard to thread safety of apr_allocator, there is still something > I do not understand. Observations: > > 1. When I remove the own allocator in h2_mplx, so it inherits the >now protected one from the master connection, all runs fine. I'm not sure this dedicated allocator is needed, mplx seems to race only with itself on ptrans (master->pool), but I may miss something. > 2. When I remove the own allocator from the slave connections also, >in h2_conn, so that slave conns also use the protected allocator >from the master conn, Hmm, rather the allocator from mplx->pool (since slave connections are children of mplx), right? > things break. E.g. strange behaviour up >to crashes under load. Is it with or without this commit? Also with 1. + 2., or 2. alone?
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
On 20.02.2017 15:55, Jim Jagielski wrote: >> On Feb 20, 2017, at 9:51 AM, Stefan Eissing >> wrote: >> >>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski : >>> >>> 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? >> So, apr_p*alloc() calls would be thread-safe if a mutex is set in >> the underlying allocator? Hmm, at what cost? would be my question. >> > The cost would be the time spent on a lock on each call to apr_palloc() > or anything that *uses* apr_palloc(). > > The idea being that if the underlying allocator has a mutex, the > assumption should be that the pool using that allocator "wants" > or "expects" to be thread-safe... It seems an easy way to create > thread-safe APR pools, but I could be missing something crucial > here. > > Of course, if the allocator does NOT have a mutex, no change and > no cost. I've always understood that creating subpools is thread safe iff the allocator has a mutex, but allocating from any single pool is not, by definition. Acquiring a mutex for every apr_palloc() seems like a good way to throw away pools' speed advantage compared to malloc(). -- Brane
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
> On Feb 20, 2017, at 9:51 AM, Stefan Eissing > wrote: > >> >> Am 20.02.2017 um 15:16 schrieb Jim Jagielski : >> >> 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? > > So, apr_p*alloc() calls would be thread-safe if a mutex is set in > the underlying allocator? Hmm, at what cost? would be my question. > The cost would be the time spent on a lock on each call to apr_palloc() or anything that *uses* apr_palloc(). The idea being that if the underlying allocator has a mutex, the assumption should be that the pool using that allocator "wants" or "expects" to be thread-safe... It seems an easy way to create thread-safe APR pools, but I could be missing something crucial here. Of course, if the allocator does NOT have a mutex, no change and no cost.
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
> Am 20.02.2017 um 15:16 schrieb Jim Jagielski : > > 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? So, apr_p*alloc() calls would be thread-safe if a mutex is set in the underlying allocator? Hmm, at what cost? would be my question. In regard to thread safety of apr_allocator, there is still something I do not understand. Observations: 1. When I remove the own allocator in h2_mplx, so it inherits the now protected one from the master connection, all runs fine. 2. When I remove the own allocator from the slave connections also, in h2_conn, so that slave conns also use the protected allocator from the master conn, things break. E.g. strange behaviour up to crashes under load. Which indicates that I have not fully understood the contract of these things, or there is a hidden assumptions in regard to conn_rec->pool. hidden to me, at least. Can someone shed light on this? > >> On Feb 20, 2017, at 8:38 AM, [email protected] 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, >> +
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
[Oups, meant to reach the list(s)]. On Mon, Feb 20, 2017 at 3:46 PM, Yann Ylavic wrote: > On Mon, Feb 20, 2017 at 3:16 PM, Jim Jagielski wrote: >> 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? > > That could be useful, but at a cost for every allocation (not only > when the active node is exhausted). > > And we don't need it for http/1 requests processing for example for > now, while it may be interesting for ptrans for requests to easily > allocate there (not anyhow of course...). > > So I'd rather use another mutex for synchronized allocations > (when/where needed by the user) with an explicit setting (at creation > time) like apr_pool_set_sync_alloc(), and then a per-call > apr_palloc_sync() maybe?
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
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, [email protected] 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); > > >
