On the 2.4.x branch:

Merge of r1779979,1780159,1780576,1780596 from trunk:
M    modules/http2/h2_bucket_beam.c
     fix for possible duplicate free of send/recv pools
M    modules/http2/h2_from_h1.c
     suppress generating responses on aborted slave connections
M    modules/http2/h2_session.c
     regression: stream ongoing streams on graceful shutdown to the end
M    modules/http2/h2_version.h



git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1780597 13f79535-47bb-0310-9956-ffa450edef68

5 files changed, 90 insertions, 64 deletions

diff --git a/CHANGES b/CHANGES
index c279d32a75..64acbbf85e 100644
--- a/CHANGES
+++ b/CHANGES
@@ -2,6 +2,10 @@
 
 Changes with Apache 2.4.26
 
+  *) mod_http2: regression fix on PR 59348, on graceful restart, ongoing 
+     streams are finished normally before the final GOAWAY is sent. 
+     [Stefan Eissing, <slavko gmail.com>]
+     
   *) mod_http2: fixes PR60599, sending proper response for conditional requests
      answered by mod_cache. [Jeff Wheelhouse, Stefan Eissing]
      
diff --git a/modules/http2/h2_bucket_beam.c b/modules/http2/h2_bucket_beam.c
index 25c2187b27..7c06e79212 100644
--- a/modules/http2/h2_bucket_beam.c
+++ b/modules/http2/h2_bucket_beam.c
@@ -429,6 +429,25 @@ static apr_status_t beam_close(h2_bucket_beam *beam)
 static void beam_set_send_pool(h2_bucket_beam *beam, apr_pool_t *pool);
 static void beam_set_recv_pool(h2_bucket_beam *beam, apr_pool_t *pool);
 
+static int pool_register(h2_bucket_beam *beam, apr_pool_t *pool, 
+                         apr_status_t (*cleanup)(void *))
+{
+    if (pool && pool != beam->pool) {
+        apr_pool_pre_cleanup_register(pool, beam, cleanup);
+        return 1;
+    }
+    return 0;
+}
+
+static int pool_kill(h2_bucket_beam *beam, apr_pool_t *pool,
+                     apr_status_t (*cleanup)(void *)) {
+    if (pool && pool != beam->pool) {
+        apr_pool_cleanup_kill(pool, beam, cleanup);
+        return 1;
+    }
+    return 0;
+}
+
 static apr_status_t beam_recv_cleanup(void *data)
 {
     h2_bucket_beam *beam = data;
@@ -440,16 +459,16 @@ static apr_status_t beam_recv_cleanup(void *data)
 
 static void beam_set_recv_pool(h2_bucket_beam *beam, apr_pool_t *pool) 
 {
-    /* if the beam owner is the sender, monitor receiver pool lifetime */ 
-    if (beam->owner == H2_BEAM_OWNER_SEND && beam->recv_pool != pool) {
-        if (beam->recv_pool) {
-            apr_pool_cleanup_kill(beam->recv_pool, beam, beam_recv_cleanup);
-        }
-        beam->recv_pool = pool;
-        if (beam->recv_pool) {
-            apr_pool_pre_cleanup_register(beam->recv_pool, beam, beam_recv_cleanup);
-        }
+    if (beam->recv_pool == pool || 
+        (beam->recv_pool && pool 
+         && apr_pool_is_ancestor(beam->recv_pool, pool))) {
+        /* when receiver same or sub-pool of existing, stick
+         * to the the pool we already have. */
+        return;
     }
+    pool_kill(beam, beam->recv_pool, beam_recv_cleanup);
+    beam->recv_pool = pool;
+    pool_register(beam, beam->recv_pool, beam_recv_cleanup);
 }
 
 static apr_status_t beam_send_cleanup(void *data)
@@ -473,65 +492,68 @@ static apr_status_t beam_send_cleanup(void *data)
 
 static void beam_set_send_pool(h2_bucket_beam *beam, apr_pool_t *pool) 
 {
-    /* if the beam owner is the receiver, monitor sender pool lifetime */
-    if (beam->owner == H2_BEAM_OWNER_RECV && beam->send_pool != pool) {
-        if (beam->send_pool && pool 
-            && apr_pool_is_ancestor(beam->send_pool, pool)) {
-            /* when sender uses sub-pools to transmit data, stick
-             * to the lifetime of the pool we already have. */
-             return;
-        }
-        if (beam->send_pool) {
-            apr_pool_cleanup_kill(beam->send_pool, beam, beam_send_cleanup);
-        }
-        beam->send_pool = pool;
-        if (beam->send_pool) {
-            apr_pool_pre_cleanup_register(beam->send_pool, beam, beam_send_cleanup);
-        }
+    if (beam->send_pool == pool || 
+        (beam->send_pool && pool 
+         && apr_pool_is_ancestor(beam->send_pool, pool))) {
+        /* when sender is same or sub-pool of existing, stick
+         * to the the pool we already have. */
+        return;
     }
+    pool_kill(beam, beam->send_pool, beam_send_cleanup);
+    beam->send_pool = pool;
+    pool_register(beam, beam->send_pool, beam_send_cleanup);
 }
 
 static apr_status_t beam_cleanup(void *data)
 {
     h2_bucket_beam *beam = data;
     apr_status_t status = APR_SUCCESS;
-    /* owner of the beam is going away, depending on its role, cleanup
-     * strategies differ. */
-    beam_close(beam);
-    switch (beam->owner) {
-        case H2_BEAM_OWNER_SEND:
-            status = beam_send_cleanup(beam);
-            beam->recv_buffer = NULL;
+    int safe_send = !beam->m_enter || (beam->owner == H2_BEAM_OWNER_SEND);
+    int safe_recv = !beam->m_enter || (beam->owner == H2_BEAM_OWNER_RECV);
+    
+    /* 
+     * Owner of the beam is going away, depending on which side it owns,
+     * cleanup strategies will differ with multi-thread protection
+     * still in place (beam->m_enter).
+     *
+     * In general, receiver holds references to memory from sender. 
+     * Clean up receiver first, if safe, then cleanup sender, if safe.
+     */
+     
+    /* When modify send is not safe, this means we still have multi-thread
+     * protection and the owner is receiving the buckets. If the sending
+     * side has not gone away, this means we could have dangling buckets
+     * in our lists that never get destroyed. This should not happen. */
+    ap_assert(safe_send || !beam->send_pool);
+    if (!H2_BLIST_EMPTY(&beam->send_list)) {
+        ap_assert(beam->send_pool);
+    }
+    
+    if (safe_recv) {
+        if (beam->recv_pool) {
+            pool_kill(beam, beam->recv_pool, beam_recv_cleanup);
             beam->recv_pool = NULL;
-            break;
-        case H2_BEAM_OWNER_RECV:
-            if (beam->recv_buffer) {
-                apr_brigade_destroy(beam->recv_buffer);
-            }
+        }
+        if (beam->recv_buffer) {
+            apr_brigade_destroy(beam->recv_buffer);
             beam->recv_buffer = NULL;
-            beam->recv_pool = NULL;
-            if (!H2_BLIST_EMPTY(&beam->send_list)) {
-                ap_assert(beam->send_pool);
-            }
-            if (beam->send_pool) {
-                /* sender has not cleaned up, its pool still lives.
-                 * this is normal if the sender uses cleanup via a bucket
-                 * such as the BUCKET_EOR for requests. In that case, the
-                 * beam should have lost its mutex protection, meaning
-                 * it is no longer used multi-threaded and we can safely
-                 * purge all remaining sender buckets. */
-                apr_pool_cleanup_kill(beam->send_pool, beam, beam_send_cleanup);
-                ap_assert(!beam->m_enter);
-                beam_send_cleanup(beam);
-            }
-            ap_assert(H2_BPROXY_LIST_EMPTY(&beam->proxies));
-            ap_assert(H2_BLIST_EMPTY(&beam->send_list));
-            ap_assert(H2_BLIST_EMPTY(&beam->hold_list));
-            ap_assert(H2_BLIST_EMPTY(&beam->purge_list));
-            break;
-        default:
-            ap_assert(NULL);
-            break;
+        }
+    }
+    else {
+        beam->recv_buffer = NULL;
+        beam->recv_pool = NULL;
+    }
+    
+    if (safe_send && beam->send_pool) {
+        pool_kill(beam, beam->send_pool, beam_send_cleanup);
+        status = beam_send_cleanup(beam);
+    }
+    
+    if (safe_recv) {
+        ap_assert(H2_BPROXY_LIST_EMPTY(&beam->proxies));
+        ap_assert(H2_BLIST_EMPTY(&beam->send_list));
+        ap_assert(H2_BLIST_EMPTY(&beam->hold_list));
+        ap_assert(H2_BLIST_EMPTY(&beam->purge_list));
     }
     return status;
 }
diff --git a/modules/http2/h2_from_h1.c b/modules/http2/h2_from_h1.c
index d3f07a1b00..a8f50987f3 100644
--- a/modules/http2/h2_from_h1.c
+++ b/modules/http2/h2_from_h1.c
@@ -517,7 +517,7 @@ apr_status_t h2_filter_headers_out(ap_filter_t *f, apr_bucket_brigade *bb)
     ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, f->c,
                   "h2_task(%s): output_filter called", task->id);
     
-    if (!task->output.sent_response) {
+    if (!task->output.sent_response && !f->c->aborted) {
         /* check, if we need to send the response now. Until we actually
          * see a DATA bucket or some EOS/EOR, we do not do so. */
         for (b = APR_BRIGADE_FIRST(bb);
diff --git a/modules/http2/h2_session.c b/modules/http2/h2_session.c
index 3dccdeec82..565207b4a4 100644
--- a/modules/http2/h2_session.c
+++ b/modules/http2/h2_session.c
@@ -2252,8 +2252,8 @@ apr_status_t h2_session_process(h2_session *session, int async)
                 }
                 else if (APR_STATUS_IS_TIMEUP(status)) {
                     /* go back to checking all inputs again */
-                    transit(session, "wait cycle", session->local.accepting? 
-                            H2_SESSION_ST_BUSY : H2_SESSION_ST_DONE);
+                    transit(session, "wait cycle", session->local.shutdown? 
+                            H2_SESSION_ST_DONE : H2_SESSION_ST_BUSY);
                 }
                 else if (APR_STATUS_IS_ECONNRESET(status) 
                          || APR_STATUS_IS_ECONNABORTED(status)) {
diff --git a/modules/http2/h2_version.h b/modules/http2/h2_version.h
index 77e29c6e70..4f33e9ab0f 100644
--- a/modules/http2/h2_version.h
+++ b/modules/http2/h2_version.h
@@ -26,7 +26,7 @@
  * @macro
  * Version number of the http2 module as c string
  */
-#define MOD_HTTP2_VERSION "1.8.10"
+#define MOD_HTTP2_VERSION "1.8.11"
 
 /**
  * @macro
@@ -34,7 +34,7 @@
  * release. This is a 24 bit number with 8 bits for major number, 8 bits
  * for minor and 8 bits for patch. Version 1.2.3 becomes 0x010203.
  */
-#define MOD_HTTP2_VERSION_NUM 0x01080a
+#define MOD_HTTP2_VERSION_NUM 0x01080b
 
 
 #endif /* mod_h2_h2_version_h */