mod_http2: fix for stream buffer handling during shutdown

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

diff --git a/CHANGES b/CHANGES
index 5285016ba68e86bdf3fb689ef5ab2e0184a83222..4a4801092a8956ca8dd84b31e77cc0ae7c213494 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -1,6 +1,9 @@
                                                          -*- coding: utf-8 -*-
 Changes with Apache 2.5.0
 
+  *) mod_http2: fixed handling of stream buffers during shutdown.
+     [Stefan Eissing]
+     
   *) mod_proxy_hcheck: Set health check URI and expression correctly for health
      check worker. PR 60038 [zdeno <zdeno@scnet.sk>]
 

diff --git a/modules/http2/h2_bucket_beam.c b/modules/http2/h2_bucket_beam.c
index 6a1e74d3e7fb5d3c532c8d18cd055c4b1227952e..1338ba68b011f3b24f6cf5495f13deefa015aa3f 100644 (file)
--- a/modules/http2/h2_bucket_beam.c
+++ b/modules/http2/h2_bucket_beam.c
@@ -550,6 +550,11 @@ apr_status_t h2_beam_shutdown(h2_bucket_beam *beam, apr_read_type_e block,
         if (clear_buffers) {
             r_purge_reds(beam);
             h2_blist_cleanup(&beam->red);
+            if (!bl.mutex && beam->green) {
+                /* not protected, may process green in red call */
+                apr_brigade_destroy(beam->green);
+                beam->green = NULL;
+            }
         }
         beam_close(beam);
         
@@ -984,6 +989,18 @@ int h2_beam_empty(h2_bucket_beam *beam)
     return empty;
 }
 
+int h2_beam_holds_proxies(h2_bucket_beam *beam)
+{
+    int has_proxies = 1;
+    h2_beam_lock bl;
+    
+    if (enter_yellow(beam, &bl) == APR_SUCCESS) {
+        has_proxies = !H2_BPROXY_LIST_EMPTY(&beam->proxies);
+        leave_yellow(beam, &bl);
+    }
+    return has_proxies;
+}
+
 int h2_beam_closed(h2_bucket_beam *beam)
 {
     return beam->closed;

diff --git a/modules/http2/h2_bucket_beam.h b/modules/http2/h2_bucket_beam.h
index fcafb063ddd441efcab6c34afcaae0c04b77e8c3..8ccd8a3aaa7f1675334744e192d8899dde237583 100644 (file)
--- a/modules/http2/h2_bucket_beam.h
+++ b/modules/http2/h2_bucket_beam.h
@@ -272,6 +272,13 @@ int h2_beam_closed(h2_bucket_beam *beam);
  */
 int h2_beam_empty(h2_bucket_beam *beam);
 
+/**
+ * Determine if beam has handed out proxy buckets that are not destroyed. 
+ * 
+ * Call from red or green side.
+ */
+int h2_beam_holds_proxies(h2_bucket_beam *beam);
+
 /**
  * Abort the beam. Will cleanup any buffered buckets and answer all send
  * and receives with APR_ECONNABORTED.

diff --git a/modules/http2/h2_mplx.c b/modules/http2/h2_mplx.c
index c1de2699b32023cba24564b2360837db63e48d58..e340c9a6788915adf0e4169784b909273874ce1c 100644 (file)
--- a/modules/http2/h2_mplx.c
+++ b/modules/http2/h2_mplx.c
@@ -197,15 +197,19 @@ static int purge_stream(void *ctx, void *val)
     h2_mplx *m = ctx;
     h2_stream *stream = val;
     int stream_id = stream->id;
-    h2_task *task = h2_ihash_get(m->tasks, stream_id);
+    h2_task *task;
+
+    /* stream_cleanup clears all buffers and destroys any buckets
+     * that might hold references into task space. Needs to be done
+     * before task destruction, otherwise it will complain. */
+    h2_stream_cleanup(stream);
     
-    h2_ihash_remove(m->spurge, stream_id);
-    h2_stream_destroy(stream);
+    task = h2_ihash_get(m->tasks, stream_id);    
     if (task) {
         task_destroy(m, task, 1);
     }
-    /* FIXME: task_destroy() might in some twisted way place the
-     * stream in the spurge hash again. Remove it last. */
+    
+    h2_stream_destroy(stream);
     h2_ihash_remove(m->spurge, stream_id);
     return 0;
 }
@@ -373,7 +377,10 @@ static void task_destroy(h2_mplx *m, h2_task *task, int called_from_master)
         if (status != APR_SUCCESS){
             ap_log_cerror(APLOG_MARK, APLOG_WARNING, status, m->c, 
                           APLOGNO(03385) "h2_task(%s): output shutdown "
-                          "incomplete", task->id);
+                          "incomplete, beam empty=%d, holds proxies=%d", 
+                          task->id,
+                          h2_beam_empty(task->output.beam),
+                          h2_beam_holds_proxies(task->output.beam));
         }
     }
     

diff --git a/modules/http2/h2_session.c b/modules/http2/h2_session.c
index e6214088aa73a197af035f76ae2c6bc59b37efef..1cb0a59f2fb21989b8f41f8b7ee31cfe3197c886 100644 (file)
--- a/modules/http2/h2_session.c
+++ b/modules/http2/h2_session.c
@@ -604,6 +604,7 @@ static int on_send_data_cb(nghttp2_session *ngh2,
         ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, session->c,
                       "h2_stream(%ld-%d): send_data_cb, reading stream",
                       session->id, (int)stream_id);
+        apr_brigade_cleanup(session->bbtmp);
         return NGHTTP2_ERR_CALLBACK_FAILURE;
     }
     else if (len != length) {
@@ -611,6 +612,7 @@ static int on_send_data_cb(nghttp2_session *ngh2,
                       "h2_stream(%ld-%d): send_data_cb, wanted %ld bytes, "
                       "got %ld from stream",
                       session->id, (int)stream_id, (long)length, (long)len);
+        apr_brigade_cleanup(session->bbtmp);
         return NGHTTP2_ERR_CALLBACK_FAILURE;
     }
     
@@ -621,8 +623,8 @@ static int on_send_data_cb(nghttp2_session *ngh2,
     }
     
     status = h2_conn_io_pass(&session->io, session->bbtmp);
-        
     apr_brigade_cleanup(session->bbtmp);
+    
     if (status == APR_SUCCESS) {
         stream->out_data_frames++;
         stream->out_data_octets += length;
@@ -775,6 +777,7 @@ static apr_status_t h2_session_shutdown(h2_session *session, int error,
     dispatch_event(session, H2_SESSION_EV_LOCAL_GOAWAY, error, msg);
     
     if (force_close) {
+        apr_brigade_cleanup(session->bbtmp);
         h2_mplx_abort(session->mplx);
     }
     
@@ -1987,6 +1990,7 @@ static void dispatch_event(h2_session *session, h2_session_event_t ev,
     }
     
     if (session->state == H2_SESSION_ST_DONE) {
+        apr_brigade_cleanup(session->bbtmp);
         h2_mplx_abort(session->mplx);
     }
 }

diff --git a/modules/http2/h2_stream.c b/modules/http2/h2_stream.c
index 9b4c017567726d79d5a69af7c467ee5f7d5eb93c..f457eb49ff8e725c41a4aef3ae182309c855f1c1 100644 (file)
--- a/modules/http2/h2_stream.c
+++ b/modules/http2/h2_stream.c
@@ -429,6 +429,7 @@ apr_status_t h2_stream_write_data(h2_stream *stream,
 {
     conn_rec *c = stream->session->c;
     apr_status_t status = APR_SUCCESS;
+    apr_bucket_brigade *tmp;
     
     AP_DEBUG_ASSERT(stream);
     if (!stream->input) {
@@ -461,18 +462,15 @@ apr_status_t h2_stream_write_data(h2_stream *stream,
         }
     }
     
-    if (!stream->tmp) {
-        stream->tmp = apr_brigade_create(stream->pool, c->bucket_alloc);
-    }
-    apr_brigade_write(stream->tmp, NULL, NULL, data, len);
+    tmp = apr_brigade_create(stream->pool, c->bucket_alloc);
+    apr_brigade_write(tmp, NULL, NULL, data, len);
     if (eos) {
-        APR_BRIGADE_INSERT_TAIL(stream->tmp, 
-                                apr_bucket_eos_create(c->bucket_alloc)); 
+        APR_BRIGADE_INSERT_TAIL(tmp, apr_bucket_eos_create(c->bucket_alloc)); 
         close_input(stream);
     }
+    status = h2_beam_send(stream->input, tmp, APR_BLOCK_READ);
+    apr_brigade_destroy(tmp);
     
-    status = h2_beam_send(stream->input, stream->tmp, APR_BLOCK_READ);
-    apr_brigade_cleanup(stream->tmp);
     stream->in_data_frames++;
     stream->in_data_octets += len;
     

diff --git a/modules/http2/h2_stream.h b/modules/http2/h2_stream.h
index 7edfae754ab6b3141389ba770ad8fee829af4ebd..4394c706f0bfb083afd4a81647e50e049d60f4f6 100644 (file)
--- a/modules/http2/h2_stream.h
+++ b/modules/http2/h2_stream.h
@@ -56,7 +56,6 @@ struct h2_stream {
     struct h2_response *last_sent;
     struct h2_bucket_beam *output;
     apr_bucket_brigade *buffer;
-    apr_bucket_brigade *tmp;
     apr_array_header_t *files;  /* apr_file_t* we collected during I/O */
 
     int rst_error;              /* stream error for RST_STREAM */

diff --git a/modules/http2/h2_task.c b/modules/http2/h2_task.c
index 75f376cf0ff4c6e549f98548a0cdef21d2ea80e0..318943ae4a83f929a7f7f8849deb44b71f926889 100644 (file)
--- a/modules/http2/h2_task.c
+++ b/modules/http2/h2_task.c
@@ -92,7 +92,7 @@ static apr_status_t input_handle_eos(h2_task *task, request_rec *r,
     apr_table_t *t = task->request? task->request->trailers : NULL;
 
     if (task->input.chunked) {
-        task->input.tmp = apr_brigade_split_ex(bb, b, task->input.tmp);
+        apr_bucket_brigade *tmp = apr_brigade_split_ex(bb, b, NULL);
         if (t && !apr_is_empty_table(t)) {
             status = apr_brigade_puts(bb, NULL, NULL, "0\r\n");
             apr_table_do(input_ser_header, task, t, NULL);
@@ -101,7 +101,8 @@ static apr_status_t input_handle_eos(h2_task *task, request_rec *r,
         else {
             status = apr_brigade_puts(bb, NULL, NULL, "0\r\n\r\n");
         }
-        APR_BRIGADE_CONCAT(bb, task->input.tmp);
+        APR_BRIGADE_CONCAT(bb, tmp);
+        apr_brigade_destroy(tmp);
     }
     else if (r && t && !apr_is_empty_table(t)){
         /* trailers passed in directly. */

diff --git a/modules/http2/h2_task.h b/modules/http2/h2_task.h
index 76e8780d2ac06d20203450f367953b2d406f6338..f13366e26cdf79a67ed7e8138ea177e4a4ee0247 100644 (file)
--- a/modules/http2/h2_task.h
+++ b/modules/http2/h2_task.h
@@ -61,7 +61,6 @@ struct h2_task {
     struct {
         struct h2_bucket_beam *beam;
         apr_bucket_brigade *bb;
-        apr_bucket_brigade *tmp;
         apr_read_type_e block;
         unsigned int chunked : 1;
         unsigned int eos : 1;

diff --git a/modules/http2/h2_version.h b/modules/http2/h2_version.h
index a21aaf9ec52c131208f6310713890877ffd66449..a495b8e2d686583d030e09c648d393acd71bab83 100644 (file)
--- 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.6.0-DEV"
+#define MOD_HTTP2_VERSION "1.6.1-DEV"
 
 /**
  * @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 0x010600
+#define MOD_HTTP2_VERSION_NUM 0x010601
 
 
 #endif /* mod_h2_h2_version_h */