v1.1.0, fixes, dynamic file handle allocation

ChangeLog

@@ -1,3 +1,32 @@
+v1.1.0
+-------------------------------------------------------------------------------- 
+ * GOAWAY will be sent when a HTTP/2 connection is shutdown, whenever the
+   connection still allows it. Calculation of the last stream id has been
+   corrected to include streams started, not only streams finished processing.
+ * several issues fixed with new fuzzing tests that simulate random closes 
+   and delays/timeouts
+ * H2SessionExtraFiles are now shared between all sessions in the same process.
+   It now works like this:
+   H2MaxWorkers * H2SessionFiles is assumed to be the total number of file
+   handles that can be safely used for HTTP/2 transfers without other parts
+   of the server running out of handles. This number is shared between all
+   HTTP/2 connections in the same server process.
+   The default is set to 2. With H2MaxWorkers on most platforms/mpms
+   defaulting to 25 that gives a maximum of 50 handles that can be invoved in
+   transfers. 
+   I think I have to write a blog post one day of how this works and affects
+   performance. tl;dr the more handles http2 may use, the faster static files
+   can be served.
+ * KeepAlive is not correctly visible on the server status page for HTTP/2
+   connections. (Would like more info there, need to extend the scoreboard
+   for it.)
+ * KeepAlive connections are *not* set aside in async MPMs such as event. This
+   is a very desirable feature, but I could not get it to work reliably with
+   the existing MPM interface. Will work on that for the next Apache release.
+   (The main problem is that event will silently close such connections and
+    http2 has no chance to send a last GOAWAY frame. That makes clients fail
+    requests which they have just started.)
+
 v1.0.18
 -------------------------------------------------------------------------------- 
  * fixed race condition in connnection shutdown that could cause indefinite 

configure.ac

@@ -14,7 +14,7 @@
 #
 
 AC_PREREQ([2.69])
-AC_INIT([mod_http2], [1.0.18], [[email protected]])
+AC_INIT([mod_http2], [1.1.0], [[email protected]])
 
 LT_PREREQ([2.2.6])
 LT_INIT()

mod_http2/h2_config.c

@@ -64,31 +64,9 @@ static h2_config defconf = {
     0,                      /* stream timeout */
 };
 
-static int files_per_session;
-
 void h2_config_init(apr_pool_t *pool)
 {
-    /* Determine a good default for this platform and mpm?
-     * TODO: not sure how APR wants to hand out this piece of 
-     * information.
-     */
-    int max_files = 256;
-    int conn_threads = 1;
-    int tx_files = max_files / 4;
-
     (void)pool;
-    ap_mpm_query(AP_MPMQ_MAX_THREADS, &conn_threads);
-    switch (h2_conn_mpm_type()) {
-        case H2_MPM_PREFORK:
-        case H2_MPM_WORKER:
-        case H2_MPM_EVENT:
-            /* allow that many transfer open files per mplx */
-            files_per_session = (tx_files / conn_threads);
-            break;
-        default:
-            /* don't know anything about it, stay safe */
-            break;
-    }
 }
 
 static void *h2_config_create(apr_pool_t *pool,
@@ -178,7 +156,6 @@ int h2_config_geti(const h2_config *conf, h2_config_var_t var)
 
 apr_int64_t h2_config_geti64(const h2_config *conf, h2_config_var_t var)
 {
-    int n;
     switch(var) {
         case H2_CONF_MAX_STREAMS:
             return H2_CONFIG_GET(conf, &defconf, h2_max_streams);
@@ -203,11 +180,7 @@ apr_int64_t h2_config_geti64(const h2_config *conf, h2_config_var_t var)
         case H2_CONF_DIRECT:
             return H2_CONFIG_GET(conf, &defconf, h2_direct);
         case H2_CONF_SESSION_FILES:
-            n = H2_CONFIG_GET(conf, &defconf, session_extra_files);
-            if (n < 0) {
-                n = files_per_session;
-            }
-            return n;
+            return H2_CONFIG_GET(conf, &defconf, session_extra_files);
         case H2_CONF_TLS_WARMUP_SIZE:
             return H2_CONFIG_GET(conf, &defconf, tls_warmup_size);
         case H2_CONF_TLS_COOLDOWN_SECS:

mod_http2/h2_conn.c

@@ -78,7 +78,7 @@ apr_status_t h2_conn_child_init(apr_pool_t *pool, server_rec *s)
 {
     const h2_config *config = h2_config_sget(s);
     apr_status_t status = APR_SUCCESS;
-    int minw, maxw;
+    int minw, maxw, max_tx_handles, n;
     int max_threads_per_child = 0;
     int idle_secs = 0;
 
@@ -105,11 +105,29 @@ apr_status_t h2_conn_child_init(apr_pool_t *pool, server_rec *s)
         maxw = minw;
     }
 
+    /* How many file handles is it safe to use for transfer
+     * to the master connection to be streamed out? 
+     * Is there a portable APR rlimit on NOFILES? Have not
+     * found it. And if, how many of those would we set aside?
+     * This leads all into a process wide handle allocation strategy
+     * which ultimately would limit the number of accepted connections
+     * with the assumption of implicitly reserving n handles for every 
+     * connection and requiring modules with excessive needs to allocate
+     * from a central pool.
+     */
+    n = h2_config_geti(config, H2_CONF_SESSION_FILES);
+    if (n < 0) {
+        max_tx_handles = maxw * 2;
+    }
+    else {
+        max_tx_handles = maxw * n;
+    }
+
     ap_log_error(APLOG_MARK, APLOG_TRACE3, 0, s,
-                 "h2_workers: min=%d max=%d, mthrpchild=%d", 
-                 minw, maxw, max_threads_per_child);
+                 "h2_workers: min=%d max=%d, mthrpchild=%d, tx_files=%d", 
+                 minw, maxw, max_threads_per_child, max_tx_handles);
+    workers = h2_workers_create(s, pool, minw, maxw, max_tx_handles);
 
-    workers = h2_workers_create(s, pool, minw, maxw);
     idle_secs = h2_config_geti(config, H2_CONF_MAX_WORKER_IDLE_SECS);
     h2_workers_set_max_idle_secs(workers, idle_secs);
 
@@ -149,9 +167,6 @@ apr_status_t h2_conn_setup(h2_ctx *ctx, conn_rec *c, request_rec *r)
     }
 
     h2_ctx_session_set(ctx, session);
-
-    ap_update_child_status_from_conn(c->sbh, SERVER_BUSY_READ, c);
-
     return APR_SUCCESS;
 }
 

mod_http2/h2_filter.c

@@ -133,7 +133,6 @@ apr_status_t h2_filter_core_input(ap_filter_t* f,
                 apr_socket_timeout_set(cin->socket, t);
             }
         }
-        ap_update_child_status_from_conn(f->c->sbh, SERVER_BUSY_READ, f->c);
         status = ap_get_brigade(f->next, cin->bb, AP_MODE_READBYTES,
                                 block, readbytes);
         if (saved_timeout != UNSET) {

mod_http2/h2_io.c

@@ -355,7 +355,7 @@ static void process_trailers(h2_io *io, apr_table_t *trailers)
 
 apr_status_t h2_io_out_write(h2_io *io, apr_bucket_brigade *bb, 
                              apr_size_t maxlen, apr_table_t *trailers,
-                             int *pfile_handles_allowed)
+                             apr_size_t *pfile_buckets_allowed)
 {
     apr_status_t status;
     int start_allowed;
@@ -397,12 +397,12 @@ apr_status_t h2_io_out_write(h2_io *io, apr_bucket_brigade *bb,
      * many open files already buffered. Otherwise we will run out of
      * file handles.
      */
-    start_allowed = *pfile_handles_allowed;
-    status = h2_util_move(io->bbout, bb, maxlen, pfile_handles_allowed, 
+    start_allowed = *pfile_buckets_allowed;
+    status = h2_util_move(io->bbout, bb, maxlen, pfile_buckets_allowed, 
                           "h2_io_out_write");
     /* track # file buckets moved into our pool */
-    if (start_allowed != *pfile_handles_allowed) {
-        io->files_handles_owned += (start_allowed - *pfile_handles_allowed);
+    if (start_allowed != *pfile_buckets_allowed) {
+        io->files_handles_owned += (start_allowed - *pfile_buckets_allowed);
     }
     return status;
 }

mod_http2/h2_io.h

@@ -158,7 +158,7 @@ apr_status_t h2_io_out_read_to(h2_io *io,
 
 apr_status_t h2_io_out_write(h2_io *io, apr_bucket_brigade *bb, 
                              apr_size_t maxlen, apr_table_t *trailers,
-                             int *pfile_buckets_allowed);
+                             apr_size_t *pfile_buckets_allowed);
 
 /**
  * Closes the input. After existing data has been read, APR_EOF will

mod_http2/h2_mplx.c

@@ -72,6 +72,28 @@ static int is_aborted(h2_mplx *m, apr_status_t *pstatus)
 
 static void have_out_data_for(h2_mplx *m, int stream_id);
 
+static void check_tx_reservation(h2_mplx *m) 
+{
+    if (m->tx_handles_reserved == 0) {
+        m->tx_handles_reserved += h2_workers_tx_reserve(m->workers, 
+            H2MIN(m->tx_chunk_size, h2_io_set_size(m->stream_ios)));
+    }
+}
+
+static void check_tx_free(h2_mplx *m) 
+{
+    if (m->tx_handles_reserved > m->tx_chunk_size) {
+        apr_size_t count = m->tx_handles_reserved - m->tx_chunk_size;
+        m->tx_handles_reserved = m->tx_chunk_size;
+        h2_workers_tx_free(m->workers, count);
+    }
+    else if (m->tx_handles_reserved 
+             && (!m->stream_ios || h2_io_set_is_empty(m->stream_ios))) {
+        h2_workers_tx_free(m->workers, m->tx_handles_reserved);
+        m->tx_handles_reserved = 0;
+    }
+}
+
 static void h2_mplx_destroy(h2_mplx *m)
 {
     AP_DEBUG_ASSERT(m);
@@ -88,6 +110,8 @@ static void h2_mplx_destroy(h2_mplx *m)
         m->stream_ios = NULL;
     }
 
+    check_tx_free(m);
+
     if (m->pool) {
         apr_pool_destroy(m->pool);
     }
@@ -142,12 +166,25 @@ h2_mplx *h2_mplx_create(conn_rec *c, apr_pool_t *parent,
         m->stream_max_mem = h2_config_geti(conf, H2_CONF_STREAM_MAX_MEM);
         m->workers = workers;
 
-        m->file_handles_allowed = h2_config_geti(conf, H2_CONF_SESSION_FILES);
+        m->tx_handles_reserved = 0;
+        m->tx_chunk_size = 4;
+
         m->stream_timeout_secs = h2_config_geti(conf, H2_CONF_STREAM_TIMEOUT_SECS);
     }
     return m;
 }
 
+int h2_mplx_get_max_stream_started(h2_mplx *m)
+{
+    int stream_id = 0;
+
+    apr_thread_mutex_lock(m->lock);
+    stream_id = m->max_stream_started;
+    apr_thread_mutex_unlock(m->lock);
+
+    return stream_id;
+}
+
 static void workers_register(h2_mplx *m)
 {
     /* Initially, there was ref count increase for this as well, but
@@ -164,11 +201,6 @@ static void workers_register(h2_mplx *m)
     h2_workers_register(m->workers, m);
 }
 
-static void workers_unregister(h2_mplx *m)
-{
-    h2_workers_unregister(m->workers, m);
-}
-
 static int io_process_events(h2_mplx *m, h2_io *io)
 {
     if (io->input_consumed && m->input_consumed) {
@@ -195,7 +227,8 @@ static void io_destroy(h2_mplx *m, h2_io *io, int events)
     /* The pool is cleared/destroyed which also closes all
      * allocated file handles. Give this count back to our
      * file handle pool. */
-    m->file_handles_allowed += io->files_handles_owned;
+    m->tx_handles_reserved += io->files_handles_owned;
+
     h2_io_set_remove(m->stream_ios, io);
     h2_io_set_remove(m->ready_ios, io);
     h2_io_destroy(io);
@@ -207,6 +240,8 @@ static void io_destroy(h2_mplx *m, h2_io *io, int events)
         }
         m->spare_pool = pool;
     }
+
+    check_tx_free(m);
 }
 
 static int io_stream_done(h2_mplx *m, h2_io *io, int rst_error) 
@@ -235,7 +270,7 @@ apr_status_t h2_mplx_release_and_join(h2_mplx *m, apr_thread_cond_t *wait)
 {
     apr_status_t status;
 
-    workers_unregister(m);
+    h2_workers_unregister(m->workers, m);
     status = apr_thread_mutex_lock(m->lock);
     if (APR_SUCCESS == status) {
         int i, wait_secs = 5;
@@ -317,11 +352,14 @@ static const h2_request *pop_request(h2_mplx *m)
 {
     const h2_request *req = NULL;
     int sid;
-    while (!req && (sid = h2_tq_shift(m->q)) > 0) {
+    while (!m->aborted && !req && (sid = h2_tq_shift(m->q)) > 0) {
         h2_io *io = h2_io_set_get(m->stream_ios, sid);
         if (io) {
             req = io->request;
             io->worker_started = 1;
+            if (sid > m->max_stream_started) {
+                m->max_stream_started = sid;
+            }
         }
     }
     return req;
@@ -613,7 +651,7 @@ static apr_status_t out_write(h2_mplx *m, h2_io *io,
            && !is_aborted(m, &status)) {
 
         status = h2_io_out_write(io, bb, m->stream_max_mem, trailers,
-                                 &m->file_handles_allowed);
+                                 &m->tx_handles_reserved);
         /* Wait for data to drain until there is room again or
          * stream timeout expires */
         h2_io_signal_init(io, H2_IO_WRITE, m->stream_timeout_secs, iowait);
@@ -654,6 +692,11 @@ static apr_status_t out_open(h2_mplx *m, int stream_id, h2_response *response,
 
         h2_io_set_response(io, response);
         h2_io_set_add(m->ready_ios, io);
+        if (response && response->http_status < 300) {
+            /* we might see some file buckets in the output, see
+             * if we have enough handles reserved. */
+            check_tx_reservation(m);
+        }
         if (bb) {
             status = out_write(m, io, f, bb, response->trailers, iowait);
         }
@@ -865,7 +908,6 @@ apr_status_t h2_mplx_reprioritize(h2_mplx *m, h2_stream_pri_cmp *cmp, void *ctx)
         }
         apr_thread_mutex_unlock(m->lock);
     }
-    workers_register(m);
     return status;
 }
 
@@ -892,6 +934,7 @@ apr_status_t h2_mplx_process(h2_mplx *m, int stream_id, const h2_request *req,
                              h2_stream_pri_cmp *cmp, void *ctx)
 {
     apr_status_t status;
+    int was_empty = 0;
 
     AP_DEBUG_ASSERT(m);
     status = apr_thread_mutex_lock(m->lock);
@@ -907,6 +950,7 @@ apr_status_t h2_mplx_process(h2_mplx *m, int stream_id, const h2_request *req,
                 status = h2_io_in_close(io);
             }
 
+            was_empty = h2_tq_empty(m->q);
             h2_tq_add(m->q, io->id, cmp, ctx);
 
             ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, m->c,
@@ -915,8 +959,7 @@ apr_status_t h2_mplx_process(h2_mplx *m, int stream_id, const h2_request *req,
         }
         apr_thread_mutex_unlock(m->lock);
     }
-
-    if (status == APR_SUCCESS) {
+    if (status == APR_SUCCESS && was_empty) {
         workers_register(m);
     }
     return status;

mod_http2/h2_mplx.h

@@ -71,6 +71,8 @@ struct h2_mplx {
     struct h2_io_set *stream_ios;
     struct h2_io_set *ready_ios;
 
+    int max_stream_started;      /* highest stream id that started processing */
+
     apr_thread_mutex_t *lock;
     struct apr_thread_cond_t *added_output;
     struct apr_thread_cond_t *join_wait;
@@ -80,7 +82,8 @@ struct h2_mplx {
 
     apr_pool_t *spare_pool;           /* spare pool, ready for next io */
     struct h2_workers *workers;
-    int file_handles_allowed;
+    apr_size_t tx_handles_reserved;
+    apr_size_t tx_chunk_size;
 
     h2_mplx_consumed_cb *input_consumed;
     void *input_consumed_ctx;
@@ -118,6 +121,14 @@ void h2_mplx_abort(h2_mplx *mplx);
 
 void h2_mplx_request_done(h2_mplx **pm, int stream_id, const struct h2_request **preq);
 
+/**
+ * Get the highest stream identifier that has been passed on to processing.
+ * Maybe 0 in case no stream has been processed yet.
+ * @param m the multiplexer
+ * @return highest stream identifier for which processing started
+ */
+int h2_mplx_get_max_stream_started(h2_mplx *m);
+
 /*******************************************************************************
  * IO lifetime of streams.
  ******************************************************************************/