Hi Stefan,

On Mon, Feb 6, 2017 at 9:57 AM, Stefan Priebe - Profihost AG
<s.pri...@profihost.ag> wrote:
>
> your last patch results in multiple crashes every second:

Sorry about that, the changes in mpm_event were incorrect (the mutex
was cleared with the pool when recycled, hence its pointer was
dangling).

New patch attached, this time tested with the httpd framework (where
the previous patch segfaulted too).

Thanks,
Yann.
Index: server/mpm/event/event.c
===================================================================
--- server/mpm/event/event.c	(revision 1781789)
+++ server/mpm/event/event.c	(working copy)
@@ -1743,6 +1743,8 @@ static void * APR_THREAD_FUNC listener_thread(apr_
                     enable_listensocks(process_slot);
                 }
                 if (!listeners_disabled) {
+                    apr_thread_mutex_t *mutex;
+
                     lr = (ap_listen_rec *) pt->baton;
                     ap_pop_pool(&ptrans, worker_queue_info);
 
@@ -1762,8 +1764,10 @@ static void * APR_THREAD_FUNC listener_thread(apr_
                             signal_threads(ST_GRACEFUL);
                             return NULL;
                         }
+                        apr_pool_tag(ptrans, "transaction");
                     }
-                    apr_pool_tag(ptrans, "transaction");
+                    apr_thread_mutex_create(&mutex, APR_THREAD_MUTEX_DEFAULT, ptrans);
+                    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);
Index: modules/http2/h2_conn.c
===================================================================
--- modules/http2/h2_conn.c	(revision 1781789)
+++ modules/http2/h2_conn.c	(working copy)
@@ -247,9 +247,10 @@ apr_status_t h2_conn_pre_close(struct h2_ctx *ctx,
     return status;
 }
 
-conn_rec *h2_slave_create(conn_rec *master, int slave_id, apr_pool_t *parent)
+conn_rec *h2_slave_create(conn_rec *master, int slave_id)
 {
     apr_allocator_t *allocator;
+    apr_thread_mutex_t *mutex;
     apr_pool_t *pool;
     conn_rec *c;
     void *cfg;
@@ -264,9 +265,11 @@ apr_status_t h2_conn_pre_close(struct h2_ctx *ctx,
      * another thread.
      */
     apr_allocator_create(&allocator);
-    apr_pool_create_ex(&pool, parent, NULL, allocator);
+    apr_pool_create_ex(&pool, master->pool, NULL, allocator);
+    apr_allocator_owner_set(allocator, pool);
     apr_pool_tag(pool, "h2_slave_conn");
-    apr_allocator_owner_set(allocator, pool);
+    apr_thread_mutex_create(&mutex, APR_THREAD_MUTEX_DEFAULT, pool);
+    apr_allocator_mutex_set(allocator, mutex);
 
     c = (conn_rec *) apr_palloc(pool, sizeof(conn_rec));
     if (c == NULL) {
Index: modules/http2/h2_conn.h
===================================================================
--- modules/http2/h2_conn.h	(revision 1781789)
+++ modules/http2/h2_conn.h	(working copy)
@@ -66,7 +66,7 @@ typedef enum {
 h2_mpm_type_t h2_conn_mpm_type(void);
 
 
-conn_rec *h2_slave_create(conn_rec *master, int slave_id, apr_pool_t *parent);
+conn_rec *h2_slave_create(conn_rec *master, int slave_id);
 void h2_slave_destroy(conn_rec *slave);
 
 apr_status_t h2_slave_run_pre_connection(conn_rec *slave, apr_socket_t *csd);
Index: modules/http2/h2_mplx.c
===================================================================
--- modules/http2/h2_mplx.c	(revision 1781789)
+++ modules/http2/h2_mplx.c	(working copy)
@@ -33,6 +33,7 @@
 #include "h2_private.h"
 #include "h2_bucket_beam.h"
 #include "h2_config.h"
+#include "h2_session.h"
 #include "h2_conn.h"
 #include "h2_ctx.h"
 #include "h2_h2.h"
@@ -245,7 +246,7 @@ static void h2_mplx_destroy(h2_mplx *m)
                   m->id, (int)h2_ihash_count(m->tasks));
     check_tx_free(m);
     /* pool will be destroyed as child of h2_session->pool,
-       slave connection pools are children of m->pool */
+       slave connection pools are children of m->c->pool */
 }
 
 /**
@@ -259,32 +260,27 @@ static void h2_mplx_destroy(h2_mplx *m)
  *   their HTTP/1 cousins, the separate allocator seems to work better
  *   than protecting a shared h2_session one with an own lock.
  */
-h2_mplx *h2_mplx_create(conn_rec *c, apr_pool_t *parent, 
-                        const h2_config *conf, 
-                        apr_interval_time_t stream_timeout,
-                        h2_workers *workers)
+h2_mplx *h2_mplx_create(h2_session *session, h2_workers *workers)
 {
     apr_status_t status = APR_SUCCESS;
-    apr_allocator_t *allocator = NULL;
+    apr_pool_t *parent = session->pool;
+    const h2_config *conf = session->config;
     h2_mplx *m;
     ap_assert(conf);
     
-    status = apr_allocator_create(&allocator);
-    if (status != APR_SUCCESS) {
-        return NULL;
-    }
-
     m = apr_pcalloc(parent, sizeof(h2_mplx));
     if (m) {
+        conn_rec *c = session->c;
+
         m->id = c->id;
         APR_RING_ELEM_INIT(m, link);
         m->c = c;
-        apr_pool_create_ex(&m->pool, parent, NULL, allocator);
+
+        apr_pool_create(&m->pool, parent);
         if (!m->pool) {
             return NULL;
         }
         apr_pool_tag(m->pool, "h2_mplx");
-        apr_allocator_owner_set(allocator, m->pool);
         
         status = apr_thread_mutex_create(&m->lock, APR_THREAD_MUTEX_DEFAULT,
                                          m->pool);
@@ -311,7 +307,7 @@ static void h2_mplx_destroy(h2_mplx *m)
         m->tasks = h2_ihash_create(m->pool, offsetof(h2_task,stream_id));
         m->redo_tasks = h2_ihash_create(m->pool, offsetof(h2_task, stream_id));
 
-        m->stream_timeout = stream_timeout;
+        m->stream_timeout = session->s->timeout;
         m->workers = workers;
         m->workers_max = workers->max_workers;
         m->workers_limit = 6; /* the original h1 max parallel connections */
@@ -891,7 +887,7 @@ static h2_task *next_stream_task(h2_mplx *m)
                 slave = *pslave;
             }
             else {
-                slave = h2_slave_create(m->c, stream->id, m->pool);
+                slave = h2_slave_create(m->c, stream->id);
                 new_conn = 1;
             }
             
Index: modules/http2/h2_mplx.h
===================================================================
--- modules/http2/h2_mplx.h	(revision 1781789)
+++ modules/http2/h2_mplx.h	(working copy)
@@ -38,7 +38,7 @@ struct apr_pool_t;
 struct apr_thread_mutex_t;
 struct apr_thread_cond_t;
 struct h2_bucket_beam;
-struct h2_config;
+struct h2_session;
 struct h2_ihash_t;
 struct h2_task;
 struct h2_stream;
@@ -124,9 +124,7 @@ apr_status_t h2_mplx_child_init(apr_pool_t *pool,
  * Create the multiplexer for the given HTTP2 session. 
  * Implicitly has reference count 1.
  */
-h2_mplx *h2_mplx_create(conn_rec *c, apr_pool_t *master, 
-                        const struct h2_config *conf, 
-                        apr_interval_time_t stream_timeout,
+h2_mplx *h2_mplx_create(struct h2_session *session,
                         struct h2_workers *workers);
 
 /**
Index: modules/http2/h2_session.c
===================================================================
--- modules/http2/h2_session.c	(revision 1781789)
+++ modules/http2/h2_session.c	(working copy)
@@ -920,8 +920,7 @@ static h2_session *h2_session_create_int(conn_rec
             return NULL;
         }
         
-        session->mplx = h2_mplx_create(c, session->pool, session->config, 
-                                       session->s->timeout, workers);
+        session->mplx = h2_mplx_create(session, workers);
         
         h2_mplx_set_consumed_cb(session->mplx, update_window, session);
         

Reply via email to