On the trunk:

mod_http2: fixed problem of forgotten requests when number of connections larger than number of workers. Some code refactor, threads now queued in slot order for vanity reasons, so that the segfaults are more likely at the top and not the bottom.

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

diff --git a/modules/http2/h2_conn.c b/modules/http2/h2_conn.c
index 7cc39ecd9373be66586870068e0cd46f1bf14f14..73c838e0e8470c00b3c93c5402d047518ff73c23 100644 (file)
--- a/modules/http2/h2_conn.c
+++ b/modules/http2/h2_conn.c
@@ -125,7 +125,11 @@ apr_status_t h2_conn_child_init(apr_pool_t *pool, server_rec *s)
         minw = max_threads_per_child;
     }
     if (maxw <= 0) {
-        maxw = minw;
+        /* As a default, this seems to work quite well under mpm_event. 
+         * For people enabling http2 under mpm_prefork, start 4 threads unless 
+         * configured otherwise. People get unhappy if their http2 requests are 
+         * blocking each other. */
+        maxw = H2MAX(3 * minw / 2, 4);
     }
     
     idle_secs = h2_config_geti(config, H2_CONF_MAX_WORKER_IDLE_SECS);

diff --git a/modules/http2/h2_mplx.c b/modules/http2/h2_mplx.c
index c1b0b049c1ba6c4988b7083f7df44e0a311996e7..7eb101db491039a9c39b57a685a123d6d53656bc 100644 (file)
--- a/modules/http2/h2_mplx.c
+++ b/modules/http2/h2_mplx.c
@@ -673,6 +673,20 @@ apr_status_t h2_mplx_reprioritize(h2_mplx *m, h2_stream_pri_cmp *cmp, void *ctx)
     return status;
 }
 
+static void register_if_needed(h2_mplx *m) 
+{
+    if (!m->is_registered && !h2_iq_empty(m->q)) {
+        apr_status_t status = h2_workers_register(m->workers, m); 
+        if (status == APR_SUCCESS) {
+            m->is_registered = 1;
+        }
+        else {
+            ap_log_cerror(APLOG_MARK, APLOG_ERR, status, m->c, APLOGNO()
+                          "h2_mplx(%ld): register at workers", m->id);
+        }
+    }
+}
+
 apr_status_t h2_mplx_process(h2_mplx *m, struct h2_stream *stream, 
                              h2_stream_pri_cmp *cmp, void *ctx)
 {
@@ -688,17 +702,13 @@ apr_status_t h2_mplx_process(h2_mplx *m, struct h2_stream *stream,
             if (h2_stream_is_ready(stream)) {
                 /* already have a response */
                 check_data_for(m, stream->id);
-                ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, m->c,
+                ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, m->c,
                               H2_STRM_MSG(stream, "process, add to readyq")); 
             }
             else {
-                h2_iq_add(m->q, stream->id, cmp, ctx);                
-                if (!m->is_registered) {
-                    if (h2_workers_register(m->workers, m) == APR_SUCCESS) {
-                        m->is_registered = 1;
-                    }
-                }
-                ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, m->c,
+                h2_iq_add(m->q, stream->id, cmp, ctx);
+                register_if_needed(m);                
+                ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, m->c,
                               H2_STRM_MSG(stream, "process, added to q")); 
             }
         }
@@ -901,11 +911,7 @@ void h2_mplx_task_done(h2_mplx *m, h2_task *task, h2_task **ptask)
             /* caller wants another task */
             *ptask = next_stream_task(m);
         }
-        if (!m->is_registered && !h2_iq_empty(m->q)) {
-            if (h2_workers_register(m->workers, m) == APR_SUCCESS) {
-                m->is_registered = 1;
-            }
-        }
+        register_if_needed(m);
         leave_mutex(m, acquired);
     }
 }
@@ -1042,6 +1048,7 @@ apr_status_t h2_mplx_idle(h2_mplx *m)
                 status = unschedule_slow_tasks(m);
             }
         }
+        register_if_needed(m);
         leave_mutex(m, acquired);
     }
     return status;

diff --git a/modules/http2/h2_workers.c b/modules/http2/h2_workers.c
index 131993495983dd4e8f728ec54ff65327fbfea3b7..fa395255e9f7a695b557af91d52a5021e14b0d43 100644 (file)
--- a/modules/http2/h2_workers.c
+++ b/modules/http2/h2_workers.c
@@ -69,6 +69,45 @@ static void push_slot(h2_slot **phead, h2_slot *slot)
     }
 }
 
+static void* APR_THREAD_FUNC slot_run(apr_thread_t *thread, void *wctx);
+
+static apr_status_t activate_slot(h2_workers *workers, h2_slot *slot) 
+{
+    apr_status_t status;
+    
+    slot->workers = workers;
+    slot->aborted = 0;
+    slot->task = NULL;
+    if (!slot->not_idle) {
+        status = apr_thread_cond_create(&slot->not_idle, workers->pool);
+        if (status != APR_SUCCESS) {
+            push_slot(&workers->free, slot);
+            return status;
+        }
+    }
+    
+    /* thread will either immediately start work or add itself
+     * to the idle queue */
+    apr_thread_create(&slot->thread, workers->thread_attr, slot_run, slot, 
+                      workers->pool);
+    if (!slot->thread) {
+        push_slot(&workers->free, slot);
+        return APR_ENOMEM;
+    }
+    
+    ++workers->worker_count;
+    return APR_SUCCESS;
+}
+
+static apr_status_t add_worker(h2_workers *workers)
+{
+    h2_slot *slot = pop_slot(&workers->free);
+    if (slot) {
+        return activate_slot(workers, slot);
+    }
+    return APR_EAGAIN;
+}
+
 static void wake_idle_worker(h2_workers *workers) 
 {
     h2_slot *slot = pop_slot(&workers->idle);
@@ -77,6 +116,9 @@ static void wake_idle_worker(h2_workers *workers)
         apr_thread_cond_signal(slot->not_idle);
         apr_thread_mutex_unlock(workers->lock);
     }
+    else if (workers->dynamic) {
+        add_worker(workers);
+    }
 }
 
 static void cleanup_zombies(h2_workers *workers)
@@ -144,12 +186,13 @@ static apr_status_t get_next(h2_slot *slot)
         }
         
         apr_thread_mutex_lock(workers->lock);
-        ++workers->idle_workers;
         cleanup_zombies(workers);
-        if (slot->next == NULL) {
-            push_slot(&workers->idle, slot);
-        }
+
+        ++workers->idle_workers;
+        push_slot(&workers->idle, slot);
         apr_thread_cond_wait(slot->not_idle, workers->lock);
+        --workers->idle_workers;
+
         apr_thread_mutex_unlock(workers->lock);
     }
     return APR_EOF;
@@ -190,36 +233,6 @@ static void* APR_THREAD_FUNC slot_run(apr_thread_t *thread, void *wctx)
     return NULL;
 }
 
-static apr_status_t activate_slot(h2_workers *workers)
-{
-    h2_slot *slot = pop_slot(&workers->free);
-    if (slot) {
-        apr_status_t status;
-        
-        slot->workers = workers;
-        slot->aborted = 0;
-        slot->task = NULL;
-        if (!slot->not_idle) {
-            status = apr_thread_cond_create(&slot->not_idle, workers->pool);
-            if (status != APR_SUCCESS) {
-                push_slot(&workers->free, slot);
-                return status;
-            }
-        }
-        
-        apr_thread_create(&slot->thread, workers->thread_attr, slot_run, slot, 
-                          workers->pool);
-        if (!slot->thread) {
-            push_slot(&workers->free, slot);
-            return APR_ENOMEM;
-        }
-
-        ++workers->worker_count;
-        return APR_SUCCESS;
-    }
-    return APR_EAGAIN;
-}
-
 static apr_status_t workers_pool_cleanup(void *data)
 {
     h2_workers *workers = data;
@@ -255,7 +268,7 @@ h2_workers *h2_workers_create(server_rec *s, apr_pool_t *server_pool,
     apr_status_t status;
     h2_workers *workers;
     apr_pool_t *pool;
-    int i;
+    int i, n;
 
     ap_assert(s);
     ap_assert(server_pool);
@@ -278,7 +291,7 @@ h2_workers *h2_workers_create(server_rec *s, apr_pool_t *server_pool,
     workers->max_workers = max_workers;
     workers->max_idle_secs = (idle_secs > 0)? idle_secs : 10;
 
-    status = h2_fifo_create(&workers->mplxs, pool, workers->max_workers);
+    status = h2_fifo_create(&workers->mplxs, pool, 2 * workers->max_workers);
     if (status != APR_SUCCESS) {
         return NULL;
     }
@@ -300,22 +313,29 @@ h2_workers *h2_workers_create(server_rec *s, apr_pool_t *server_pool,
                                      APR_THREAD_MUTEX_DEFAULT,
                                      workers->pool);
     if (status == APR_SUCCESS) {        
-        int n = workers->nslots = workers->max_workers;
+        n = workers->nslots = workers->max_workers;
         workers->slots = apr_pcalloc(workers->pool, n * sizeof(h2_slot));
         if (workers->slots == NULL) {
+            workers->nslots = 0;
             status = APR_ENOMEM;
         }
-    }
-    if (status == APR_SUCCESS) {        
-        workers->free = &workers->slots[0];
-        for (i = 0; i < workers->nslots-1; ++i) {
-            workers->slots[i].next = &workers->slots[i+1];
+        for (i = 0; i < n; ++i) {
             workers->slots[i].id = i;
         }
-        while (workers->worker_count < workers->max_workers 
-               && status == APR_SUCCESS) {
-            status = activate_slot(workers);
+    }
+    if (status == APR_SUCCESS) {
+        /* we activate all for now, TODO: support min_workers again.
+         * do this in reverse for vanity reasons so slot 0 will most
+         * likely be at head of idle queue. */
+        n = workers->max_workers;
+        for (i = n-1; i >= 0; --i) {
+            status = activate_slot(workers, &workers->slots[i]);
+        }
+        /* the rest of the slots go on the free list */
+        for(i = n; i < workers->nslots; ++i) {
+            push_slot(&workers->free, &workers->slots[i]);
         }
+        workers->dynamic = (workers->worker_count < workers->max_workers);
     }
     if (status == APR_SUCCESS) {
         apr_pool_pre_cleanup_register(pool, workers, workers_pool_cleanup);    
@@ -326,11 +346,7 @@ h2_workers *h2_workers_create(server_rec *s, apr_pool_t *server_pool,
 
 apr_status_t h2_workers_register(h2_workers *workers, struct h2_mplx *m)
 {
-    apr_status_t status;
-    if ((status = h2_fifo_try_push(workers->mplxs, m)) != APR_SUCCESS) {
-        ap_log_error(APLOG_MARK, APLOG_TRACE3, status, workers->s,
-                     "h2_workers: unable to push mplx(%ld)", m->id);
-    } 
+    apr_status_t status = h2_fifo_push(workers->mplxs, m);
     wake_idle_worker(workers);
     return status;
 }

diff --git a/modules/http2/h2_workers.h b/modules/http2/h2_workers.h
index 3035ab3a81a7983f992222e30364c6ce120ee5a0..30a7514cd0cdf2028dc420b0902140f09a60ddfe 100644 (file)
--- a/modules/http2/h2_workers.h
+++ b/modules/http2/h2_workers.h
@@ -44,7 +44,8 @@ struct h2_workers {
     int idle_workers;
     int max_idle_secs;
     
-    unsigned int aborted : 1;
+    int aborted;
+    int dynamic;
 
     apr_threadattr_t *thread_attr;
     int nslots;