On Tue, Feb 21, 2017 at 12:47 PM, Jim Jagielski <j...@jagunet.com> wrote:
>
>> On Feb 20, 2017, at 3:54 PM, Ruediger Pluem <rpl...@apache.org> wrote:
>>
>>
>>
>> On 02/20/2017 02:38 PM, 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.
>>
>> 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);
