On the trunk:

M    modules/http2/h2_bucket_beam.c
     - renaming: former red/green is now send/recv, better to read
M    modules/http2/h2_from_h1.c
     - produce a response also when body it totally absent (PR 60599)
M    modules/http2/h2_session.c
     - more work on cleaner connection shutdown



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

3 files changed, 70 insertions, 90 deletions

diff --git a/modules/http2/h2_bucket_beam.c b/modules/http2/h2_bucket_beam.c
index 30b9ffdec5..25c2187b27 100644
--- a/modules/http2/h2_bucket_beam.c
+++ b/modules/http2/h2_bucket_beam.c
@@ -361,7 +361,7 @@ static void h2_beam_emitted(h2_bucket_beam *beam, h2_beam_proxy *proxy)
             if (b != H2_BLIST_SENTINEL(&beam->hold_list)) {
                 /* bucket is in hold as it should be, mark this one
                  * and all before it for purging. We might have placed meta
-                 * buckets without a green proxy into the hold before it 
+                 * buckets without a receiver proxy into the hold before it 
                  * and schedule them for purging now */
                 for (b = H2_BLIST_FIRST(&beam->hold_list); 
                      b != H2_BLIST_SENTINEL(&beam->hold_list);
@@ -455,7 +455,7 @@ static void beam_set_recv_pool(h2_bucket_beam *beam, apr_pool_t *pool)
 static apr_status_t beam_send_cleanup(void *data)
 {
     h2_bucket_beam *beam = data;
-    /* sender has gone away, clear up all references to its memory */
+    /* sender is going away, clear up all references to its memory */
     r_purge_sent(beam);
     h2_blist_cleanup(&beam->send_list);
     report_consumption(beam);
@@ -740,7 +740,7 @@ static apr_status_t append_bucket(h2_bucket_beam *beam,
     
 
     /* The fundamental problem is that reading a sender bucket from
-     * a green thread is a total NO GO, because the bucket might use
+     * a receiver thread is a total NO GO, because the bucket might use
      * its pool/bucket_alloc from a foreign thread and that will
      * corrupt. */
     status = APR_ENOTIMPL;
@@ -751,7 +751,7 @@ static apr_status_t append_bucket(h2_bucket_beam *beam,
         status = apr_bucket_setaside(b, beam->send_pool);
     }
     else if (APR_BUCKET_IS_HEAP(b)) {
-        /* For heap buckets read from a green thread is fine. The
+        /* For heap buckets read from a receiver thread is fine. The
          * data will be there and live until the bucket itself is
          * destroyed. */
         status = APR_SUCCESS;
@@ -760,7 +760,7 @@ static apr_status_t append_bucket(h2_bucket_beam *beam,
         /* pool buckets are bastards that register at pool cleanup
          * to morph themselves into heap buckets. That may happen anytime,
          * even after the bucket data pointer has been read. So at
-         * any time inside the green thread, the pool bucket memory
+         * any time inside the receiver thread, the pool bucket memory
          * may disappear. yikes. */
         status = apr_bucket_read(b, &data, &len, APR_BLOCK_READ);
         if (status == APR_SUCCESS) {
@@ -879,13 +879,13 @@ apr_status_t h2_beam_receive(h2_bucket_beam *beam,
                              apr_off_t readbytes)
 {
     h2_beam_lock bl;
-    apr_bucket *bsender, *bgreen, *ng;
+    apr_bucket *bsender, *brecv, *ng;
     int transferred = 0;
     apr_status_t status = APR_SUCCESS;
     apr_off_t remain = readbytes;
     int transferred_buckets = 0;
     
-    /* Called from the green thread to take buckets from the beam */
+    /* Called from the receiver thread to take buckets from the beam */
     if (enter_yellow(beam, &bl) == APR_SUCCESS) {
 transfer:
         if (beam->aborted) {
@@ -896,26 +896,26 @@ transfer:
             goto leave;
         }
 
-        /* transfer enough buckets from our green brigade, if we have one */
+        /* transfer enough buckets from our receiver brigade, if we have one */
         beam_set_recv_pool(beam, bb->p);
         while (beam->recv_buffer
                && !APR_BRIGADE_EMPTY(beam->recv_buffer)
                && (readbytes <= 0 || remain >= 0)) {
-            bgreen = APR_BRIGADE_FIRST(beam->recv_buffer);
-            if (readbytes > 0 && bgreen->length > 0 && remain <= 0) {
+            brecv = APR_BRIGADE_FIRST(beam->recv_buffer);
+            if (readbytes > 0 && brecv->length > 0 && remain <= 0) {
                 break;
             }            
-            APR_BUCKET_REMOVE(bgreen);
-            APR_BRIGADE_INSERT_TAIL(bb, bgreen);
-            remain -= bgreen->length;
+            APR_BUCKET_REMOVE(brecv);
+            APR_BRIGADE_INSERT_TAIL(bb, brecv);
+            remain -= brecv->length;
             ++transferred;
         }
 
         /* transfer from our sender brigade, transforming sender buckets to
-         * green ones until we have enough */
+         * receiver ones until we have enough */
         while (!H2_BLIST_EMPTY(&beam->send_list) && (readbytes <= 0 || remain >= 0)) {
             bsender = H2_BLIST_FIRST(&beam->send_list);
-            bgreen = NULL;
+            brecv = NULL;
             
             if (readbytes > 0 && bsender->length > 0 && remain <= 0) {
                 break;
@@ -923,15 +923,15 @@ transfer:
                         
             if (APR_BUCKET_IS_METADATA(bsender)) {
                 if (APR_BUCKET_IS_EOS(bsender)) {
-                    bgreen = apr_bucket_eos_create(bb->bucket_alloc);
+                    brecv = apr_bucket_eos_create(bb->bucket_alloc);
                     beam->close_sent = 1;
                 }
                 else if (APR_BUCKET_IS_FLUSH(bsender)) {
-                    bgreen = apr_bucket_flush_create(bb->bucket_alloc);
+                    brecv = apr_bucket_flush_create(bb->bucket_alloc);
                 }
                 else if (AP_BUCKET_IS_ERROR(bsender)) {
                     ap_bucket_error *eb = (ap_bucket_error *)bsender;
-                    bgreen = ap_bucket_error_create(eb->status, eb->data,
+                    brecv = ap_bucket_error_create(eb->status, eb->data,
                                                     bb->p, bb->bucket_alloc);
                 }
             }
@@ -966,33 +966,33 @@ transfer:
                 continue;
             }
             else {
-                /* create a "green" standin bucket. we took care about the
+                /* create a "receiver" standin bucket. we took care about the
                  * underlying sender bucket and its data when we placed it into
                  * the sender brigade.
                  * the beam bucket will notify us on destruction that bsender is
                  * no longer needed. */
-                bgreen = h2_beam_bucket_create(beam, bsender, bb->bucket_alloc,
+                brecv = h2_beam_bucket_create(beam, bsender, bb->bucket_alloc,
                                                beam->buckets_sent++);
             }
             
             /* Place the sender bucket into our hold, to be destroyed when no
-             * green bucket references it any more. */
+             * receiver bucket references it any more. */
             APR_BUCKET_REMOVE(bsender);
             H2_BLIST_INSERT_TAIL(&beam->hold_list, bsender);
             beam->received_bytes += bsender->length;
             ++transferred_buckets;
             
-            if (bgreen) {
-                APR_BRIGADE_INSERT_TAIL(bb, bgreen);
-                remain -= bgreen->length;
+            if (brecv) {
+                APR_BRIGADE_INSERT_TAIL(bb, brecv);
+                remain -= brecv->length;
                 ++transferred;
             }
             else {
-                bgreen = h2_beam_bucket(beam, bb, bsender);
-                while (bgreen && bgreen != APR_BRIGADE_SENTINEL(bb)) {
+                brecv = h2_beam_bucket(beam, bb, bsender);
+                while (brecv && brecv != APR_BRIGADE_SENTINEL(bb)) {
                     ++transferred;
-                    remain -= bgreen->length;
-                    bgreen = APR_BUCKET_NEXT(bgreen);
+                    remain -= brecv->length;
+                    brecv = APR_BUCKET_NEXT(brecv);
                 }
             }
         }
@@ -1000,14 +1000,14 @@ transfer:
         if (readbytes > 0 && remain < 0) {
             /* too much, put some back */
             remain = readbytes;
-            for (bgreen = APR_BRIGADE_FIRST(bb);
-                 bgreen != APR_BRIGADE_SENTINEL(bb);
-                 bgreen = APR_BUCKET_NEXT(bgreen)) {
-                 remain -= bgreen->length;
+            for (brecv = APR_BRIGADE_FIRST(bb);
+                 brecv != APR_BRIGADE_SENTINEL(bb);
+                 brecv = APR_BUCKET_NEXT(brecv)) {
+                 remain -= brecv->length;
                  if (remain < 0) {
-                     apr_bucket_split(bgreen, bgreen->length+remain);
+                     apr_bucket_split(brecv, brecv->length+remain);
                      beam->recv_buffer = apr_brigade_split_ex(bb, 
-                                                        APR_BUCKET_NEXT(bgreen), 
+                                                        APR_BUCKET_NEXT(brecv), 
                                                         beam->recv_buffer);
                      break;
                  }
diff --git a/modules/http2/h2_from_h1.c b/modules/http2/h2_from_h1.c
index 64cfb76778..fbcf483ef3 100644
--- a/modules/http2/h2_from_h1.c
+++ b/modules/http2/h2_from_h1.c
@@ -527,6 +527,10 @@ apr_status_t h2_filter_headers_out(ap_filter_t *f, apr_bucket_brigade *bb)
             if (AP_BUCKET_IS_ERROR(b) && !eb) {
                 eb = b->data;
             }
+            else if (APR_BUCKET_IS_EOS(b) || AP_BUCKET_IS_EOR(b)) {
+                body_bucket = b;
+                break;
+            } 
             else if (AP_BUCKET_IS_EOC(b)) {
                 /* If we see an EOC bucket it is a signal that we should get out
                  * of the way doing nothing.
diff --git a/modules/http2/h2_session.c b/modules/http2/h2_session.c
index 053bea94ba..3dccdeec82 100644
--- a/modules/http2/h2_session.c
+++ b/modules/http2/h2_session.c
@@ -95,7 +95,7 @@ typedef struct stream_sel_ctx {
     h2_stream *candidate;
 } stream_sel_ctx;
 
-static int find_cleanup_stream(h2_stream *stream, void *ictx)
+static int find_unprocessed_stream(h2_stream *stream, void *ictx)
 {
     stream_sel_ctx *ctx = ictx;
     if (H2_STREAM_CLIENT_INITIATED(stream->id)) {
@@ -115,20 +115,17 @@ static int find_cleanup_stream(h2_stream *stream, void *ictx)
     return 1;
 }
 
-static void cleanup_streams(h2_session *session)
+static void cleanup_unprocessed_streams(h2_session *session)
 {
     stream_sel_ctx ctx;
     ctx.session = session;
-    ctx.candidate = NULL;
     while (1) {
-        h2_mplx_stream_do(session->mplx, find_cleanup_stream, &ctx);
-        if (ctx.candidate) {
-            h2_session_stream_done(session, ctx.candidate);
-            ctx.candidate = NULL;
-        }
-        else {
+        ctx.candidate = NULL;
+        h2_mplx_stream_do(session->mplx, find_unprocessed_stream, &ctx);
+        if (!ctx.candidate) {
             break;
         }
+        h2_session_stream_done(session, ctx.candidate);
     }
 }
 
@@ -317,9 +314,9 @@ static int on_data_chunk_recv_cb(nghttp2_session *ngh2, uint8_t flags,
     return 0;
 }
 
-static apr_status_t stream_release(h2_session *session, 
-                                   h2_stream *stream,
-                                   uint32_t error_code) 
+static apr_status_t stream_closed(h2_session *session, 
+                                  h2_stream *stream,
+                                  uint32_t error_code) 
 {
     conn_rec *c = session->c;
     apr_bucket *b;
@@ -329,10 +326,9 @@ static apr_status_t stream_release(h2_session *session,
         ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c,
                       "h2_stream(%ld-%d): handled, closing", 
                       session->id, (int)stream->id);
-        if (H2_STREAM_CLIENT_INITIATED(stream->id)) {
-            if (stream->id > session->local.completed_max) {
-                session->local.completed_max = stream->id;
-            }
+        if (H2_STREAM_CLIENT_INITIATED(stream->id)
+            && stream->id > session->local.completed_max) {
+            session->local.completed_max = stream->id;
         }
     }
     else {
@@ -342,7 +338,11 @@ static apr_status_t stream_release(h2_session *session,
                       h2_h2_err_description(error_code));
         h2_stream_rst(stream, error_code);
     }
-    
+    /* The stream might have data in the buffers of the main connection.
+     * We can only free the allocated resources once all had been written.
+     * Send a special buckets on the connection that gets destroyed when
+     * all preceding data has been handled. On its destruction, it is safe
+     * to purge all resources of the stream. */
     b = h2_bucket_eos_create(c->bucket_alloc, stream);
     APR_BRIGADE_INSERT_TAIL(session->bbtmp, b);
     status = h2_conn_io_pass(&session->io, session->bbtmp);
@@ -359,7 +359,7 @@ static int on_stream_close_cb(nghttp2_session *ngh2, int32_t stream_id,
     (void)ngh2;
     stream = get_stream(session, stream_id);
     if (stream) {
-        stream_release(session, stream, error_code);
+        stream_closed(session, stream, error_code);
     }
     return 0;
 }
@@ -729,32 +729,6 @@ static apr_status_t init_callbacks(conn_rec *c, nghttp2_session_callbacks **pcb)
     return APR_SUCCESS;
 }
 
-static void h2_session_cleanup(h2_session *session)
-{
-    ap_assert(session);    
-
-    if (session->mplx) {
-        h2_mplx_set_consumed_cb(session->mplx, NULL, NULL);
-        h2_mplx_release_and_join(session->mplx, session->iowait);
-        session->mplx = NULL;
-    }
-
-    ap_remove_input_filter_byhandle((session->r? session->r->input_filters :
-                                     session->c->input_filters), "H2_IN");
-    if (session->ngh2) {
-        nghttp2_session_del(session->ngh2);
-        session->ngh2 = NULL;
-    }
-    if (session->c) {
-        h2_ctx_clear(session->c);
-    }
-
-    if (APLOGctrace1(session->c)) {
-        ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, session->c,
-                      "h2_session(%ld): cleanup", session->id);
-    }
-}
-
 static apr_status_t h2_session_shutdown_notice(h2_session *session)
 {
     apr_status_t status;
@@ -829,17 +803,11 @@ static apr_status_t h2_session_shutdown(h2_session *session, int error,
 static apr_status_t session_pool_cleanup(void *data)
 {
     h2_session *session = data;
-    /* On a controlled connection shutdown, this gets never
-     * called as we deregister and destroy our pool manually.
-     * However when we have an async mpm, and handed it our idle
-     * connection, it will just cleanup once the connection is closed
-     * from the other side (and sometimes even from out side) and
-     * here we arrive then.
-     */
     ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, session->c,
                   "session(%ld): pool_cleanup", session->id);
     
-    if (session->state != H2_SESSION_ST_DONE) {
+    if (session->state != H2_SESSION_ST_DONE
+        && session->state != H2_SESSION_ST_INIT) {
         /* Not good. The connection is being torn down and we have
          * not sent a goaway. This is considered a protocol error and
          * the client has to assume that any streams "in flight" may have
@@ -853,8 +821,16 @@ static apr_status_t session_pool_cleanup(void *data)
                       "goodbye, clients will be confused, should not happen", 
                       session->id);
     }
-    h2_session_cleanup(session);
-    session->pool = NULL;
+
+    if (session->mplx) {
+        h2_mplx_set_consumed_cb(session->mplx, NULL, NULL);
+        h2_mplx_release_and_join(session->mplx, session->iowait);
+        session->mplx = NULL;
+    }
+    if (session->ngh2) {
+        nghttp2_session_del(session->ngh2);
+        session->ngh2 = NULL;
+    }
     return APR_SUCCESS;
 }
 
@@ -1774,7 +1750,7 @@ static void h2_session_ev_init(h2_session *session, int arg, const char *msg)
 
 static void h2_session_ev_local_goaway(h2_session *session, int arg, const char *msg)
 {
-    cleanup_streams(session);
+    cleanup_unprocessed_streams(session);
     if (!session->remote.shutdown) {
         update_child_status(session, SERVER_CLOSING, "local goaway");
     }
@@ -1787,7 +1763,7 @@ static void h2_session_ev_remote_goaway(h2_session *session, int arg, const char
         session->remote.error = arg;
         session->remote.accepting = 0;
         session->remote.shutdown = 1;
-        cleanup_streams(session);
+        cleanup_unprocessed_streams(session);
         update_child_status(session, SERVER_CLOSING, "remote goaway");
         transit(session, "remote goaway", H2_SESSION_ST_DONE);
     }