On the turnk:

mod_http2: fixed two deadlocks introduced by removing nested mplx locking earlier.

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1790754 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/modules/http2/h2_bucket_beam.c b/modules/http2/h2_bucket_beam.c
index 17ad3d95f139b178eb64bbac4424ce5f3d0b6556..872c67544d09dd140a623a508aea91335fe367c8 100644 (file)
--- a/modules/http2/h2_bucket_beam.c
+++ b/modules/http2/h2_bucket_beam.c
@@ -1035,7 +1035,11 @@ transfer:
                 ++transferred;
             }
             else {
+                /* let outside hook determine how bucket is beamed */
+                leave_yellow(beam, &bl);
                 brecv = h2_beam_bucket(beam, bb, bsender);
+                enter_yellow(beam, &bl);
+                
                 while (brecv && brecv != APR_BRIGADE_SENTINEL(bb)) {
                     ++transferred;
                     remain -= brecv->length;

diff --git a/modules/http2/h2_mplx.c b/modules/http2/h2_mplx.c
index 40af1c60001a2f7e63a9730e7a6ebc5cb0d21ae9..73019faeebdb3bb757a99748bd2c661c19ada8f8 100644 (file)
--- a/modules/http2/h2_mplx.c
+++ b/modules/http2/h2_mplx.c
@@ -319,12 +319,14 @@ static int stream_destroy_iter(void *ctx, void *val)
     return 0;
 }
 
-static void purge_streams(h2_mplx *m)
+static void purge_streams(h2_mplx *m, int lock)
 {
     if (!h2_ihash_empty(m->spurge)) {
+        H2_MPLX_ENTER_MAYBE(m, lock);
         while (!h2_ihash_iter(m->spurge, stream_destroy_iter, m)) {
             /* repeat until empty */
         }
+        H2_MPLX_LEAVE_MAYBE(m, lock);
     }
 }
 
@@ -587,7 +589,7 @@ apr_status_t h2_mplx_out_trywait(h2_mplx *m, apr_interval_time_t timeout,
         status = APR_SUCCESS;
     }
     else {
-        purge_streams(m);
+        purge_streams(m, 0);
         h2_ihash_iter(m->streams, report_consumption_iter, m);
         m->added_output = iowait;
         status = apr_thread_cond_timedwait(m->added_output, m->lock, timeout);
@@ -816,35 +818,37 @@ static void task_done(h2_mplx *m, h2_task *task, h2_req_engine *ngn)
     }
     
     stream = h2_ihash_get(m->streams, task->stream_id);
-    if (stream && !m->aborted && h2_ihash_get(m->sredo, stream->id)) {
-        /* reset and schedule again */
-        h2_task_redo(task);
-        h2_ihash_remove(m->sredo, stream->id);
-        h2_iq_add(m->q, stream->id, NULL, NULL);
-        return;
-    }
-    
     if (stream) {
-        /* stream not cleaned up, stay around */
-        ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, m->c,
-                      H2_STRM_MSG(stream, "task_done, stream open")); 
-        /* more data will not arrive, resume the stream */
-        if (stream->input) {
-            h2_beam_mutex_disable(stream->input);
-            h2_beam_leave(stream->input);
+        /* stream not done yet. */
+        if (!m->aborted && h2_ihash_get(m->sredo, stream->id)) {
+            /* reset and schedule again */
+            h2_task_redo(task);
+            h2_ihash_remove(m->sredo, stream->id);
+            h2_iq_add(m->q, stream->id, NULL, NULL);
         }
-        if (stream->output) {
-            h2_beam_mutex_disable(stream->output);
+        else {
+            /* stream not cleaned up, stay around */
+            ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, m->c,
+                          H2_STRM_MSG(stream, "task_done, stream open")); 
+            /* more data will not arrive, resume the stream */
+            check_data_for(m, stream, 0);
+            
+            if (stream->input) {
+                h2_beam_leave(stream->input);
+                h2_beam_mutex_disable(stream->input);
+            }
+            if (stream->output) {
+                h2_beam_mutex_disable(stream->output);
+            }
         }
-        check_data_for(m, stream, 0);
     }
     else if ((stream = h2_ihash_get(m->shold, task->stream_id)) != NULL) {
+        /* stream is done, was just waiting for this. */
         ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, m->c,
                       H2_STRM_MSG(stream, "task_done, in hold"));
-        /* stream was just waiting for us. */
         if (stream->input) {
-            h2_beam_mutex_disable(stream->input);
             h2_beam_leave(stream->input);
+            h2_beam_mutex_disable(stream->input);
         }
         if (stream->output) {
             h2_beam_mutex_disable(stream->output);
@@ -870,6 +874,7 @@ void h2_mplx_task_done(h2_mplx *m, h2_task *task, h2_task **ptask)
 
     task_done(m, task, NULL);
     --m->tasks_active;
+    
     if (m->join_wait) {
         apr_thread_cond_signal(m->join_wait);
     }
@@ -1145,6 +1150,7 @@ void h2_mplx_req_engine_done(h2_req_engine *ngn, conn_rec *r_conn,
             && !h2_ihash_get(m->sredo, stream->id)) {
             h2_ihash_add(m->sredo, stream);
         }
+
         if (task->engine) { 
             /* cannot report that as done until engine returns */
         }
@@ -1177,7 +1183,8 @@ apr_status_t h2_mplx_dispatch_master_events(h2_mplx *m,
     apr_atomic_set32(&m->event_pending, 0);
 
     /* update input windows for streams */
-    h2_ihash_iter(m->streams, report_consumption_iter, m);
+    h2_ihash_iter(m->streams, report_consumption_iter, m);    
+    purge_streams(m, 1);
     
     n = h2_fifo_count(m->readyq);
     while (n > 0 
@@ -1186,10 +1193,6 @@ apr_status_t h2_mplx_dispatch_master_events(h2_mplx *m,
         on_resume(on_ctx, stream);
     }
     
-    H2_MPLX_ENTER(m);
-    purge_streams(m);
-    H2_MPLX_LEAVE(m);
-    
     return APR_SUCCESS;
 }
 

diff --git a/modules/http2/h2_task.c b/modules/http2/h2_task.c
index 5ab485faab3c0afe07aa2d31d4e69a414212874e..1ef0d9a887a49d6962a873622e4ae4985ba353e6 100644 (file)
--- a/modules/http2/h2_task.c
+++ b/modules/http2/h2_task.c
@@ -383,7 +383,7 @@ static apr_status_t h2_filter_parse_h1(ap_filter_t* f, apr_bucket_brigade* bb)
     /* There are cases where we need to parse a serialized http/1.1 
      * response. One example is a 100-continue answer in serialized mode
      * or via a mod_proxy setup */
-    while (!task->output.sent_response) {
+    while (bb && !task->output.sent_response) {
         status = h2_from_h1_parse_response(task, f, bb);
         ap_log_cerror(APLOG_MARK, APLOG_TRACE2, status, f->c,
                       "h2_task(%s): parsed response", task->id);

diff --git a/modules/http2/h2_util.c b/modules/http2/h2_util.c
index a0b81fa0a8cd4a7827b599aa546ff7fd52d05f36..0ac65ccf656b1a6a7f446174fc5df32348ad0311 100644 (file)
--- a/modules/http2/h2_util.c
+++ b/modules/http2/h2_util.c
@@ -793,6 +793,22 @@ apr_status_t h2_fifo_try_push(h2_fifo *fifo, void *elem)
     return fifo_push(fifo, elem, 0);
 }
 
+static void *pull_head(h2_fifo *fifo)
+{
+    void *elem;
+    
+    ap_assert(fifo->count > 0);
+    elem = fifo->elems[fifo->head];
+    --fifo->count;
+    if (fifo->count > 0) {
+        fifo->head = nth_index(fifo, 1);
+        if (fifo->count+1 == fifo->nelems) {
+            apr_thread_cond_broadcast(fifo->not_full);
+        }
+    }
+    return elem;
+}
+
 static apr_status_t fifo_pull(h2_fifo *fifo, void **pelem, int block)
 {
     apr_status_t rv;
@@ -809,14 +825,8 @@ static apr_status_t fifo_pull(h2_fifo *fifo, void **pelem, int block)
         }
 
         ap_assert(fifo->count > 0);
-        *pelem = fifo->elems[fifo->head];
-        --fifo->count;
-        if (fifo->count > 0) {
-            fifo->head = nth_index(fifo, 1);
-            if (fifo->count+1 == fifo->nelems) {
-                apr_thread_cond_broadcast(fifo->not_full);
-            }
-        }
+        *pelem = pull_head(fifo);
+
         apr_thread_mutex_unlock(fifo->lock);
     }
     return rv;
@@ -848,29 +858,18 @@ static apr_status_t fifo_peek(h2_fifo *fifo, h2_fifo_peek_fn *fn, void *ctx, int
         }
 
         ap_assert(fifo->count > 0);
-        elem = fifo->elems[fifo->head];
+        elem = pull_head(fifo);
         
+        apr_thread_mutex_unlock(fifo->lock);
+
         switch (fn(elem, ctx)) {
             case H2_FIFO_OP_PULL:
-                --fifo->count;
-                if (fifo->count > 0) {
-                    fifo->head = nth_index(fifo, 1);
-                    if (fifo->count+1 == fifo->nelems) {
-                        apr_thread_cond_broadcast(fifo->not_full);
-                    }
-                }
                 break;
             case H2_FIFO_OP_REPUSH:
-                if (fifo->count > 1) {
-                    fifo->head = nth_index(fifo, 1);
-                    if (fifo->count < fifo->nelems) {
-                        fifo->elems[nth_index(fifo, fifo->count-1)] = elem;
-                    }
-                }
+                return h2_fifo_push(fifo, elem);
                 break;
         }
         
-        apr_thread_mutex_unlock(fifo->lock);
     }
     return rv;
 }

diff --git a/modules/http2/h2_workers.c b/modules/http2/h2_workers.c
index d14502341a011bff773703b5760f3a8fe07354db..9c7afc64e6c0750882810e1f0a81c7fcfae3d10d 100644 (file)
--- a/modules/http2/h2_workers.c
+++ b/modules/http2/h2_workers.c
@@ -249,8 +249,7 @@ static apr_status_t workers_pool_cleanup(void *data)
     
     if (!workers->aborted) {
         workers->aborted = 1;
-        /* before we go, cleanup any zombies and abort the rest */
-        cleanup_zombies(workers);
+        /* abort all idle slots */
         for (;;) {
             slot = pop_slot(&workers->idle);
             if (slot) {
@@ -266,6 +265,8 @@ static apr_status_t workers_pool_cleanup(void *data)
 
         h2_fifo_term(workers->mplxs);
         h2_fifo_interrupt(workers->mplxs);
+
+        cleanup_zombies(workers);
     }
     return APR_SUCCESS;
 }