On the trunk:

Fix mod_h2/github issue #126: correct lifetime of data sent on temp pools

* modules/http2/h2_bucket_beam.c
  - ignore send pools that are sub-pools of the existing one
  - added h2_beam_send_from() to allow explicit registering of the
    correct pool for the sending

* modules/http2/h2_bucket_beam.h
  - add prototype for h2_beam_send_from()

* modules/http2/h2_mplx.c
  - adding logging of output beam state

* modules/http2/h2_stream.c
  - register stream pool for sending data on input beam

* modules/http2/h2_task.c
  - register task pool on output beam on creation
  - adding trace logging

* modules/http2/h2_proxy_session.c
  - fixing a type in a comment while we're at it

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

diff --git a/CHANGES b/CHANGES
index 92e1c4bdf3c6d7077b00f13e7a0b4345c801629d..36fbe9c2a37dab5ad1c52361d48ccbe0ddfee246 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -1,6 +1,10 @@
                                                          -*- coding: utf-8 -*-
 Changes with Apache 2.5.0
 
+  *) mod_http2: fixes https://github.com/icing/mod_h2/issues/126 e.g. beam
+     bucket lifetime handling when data is sent over temporary pools.
+     [Stefan Eissing] 
+  
   *) mod_proxy_{ajp,fcgi}: Fix a possible crash when reusing an established
      backend connection, happening with LogLevel trace2 or higher configured,
      or at any log level with compilers not detected as C99 compliant (e.g.

diff --git a/modules/http2/h2_bucket_beam.c b/modules/http2/h2_bucket_beam.c
index 9b9c3b3a2f482cec14f0d7b6e2fed4d57bed9860..3d78a7937d778a9a591bb13ab3cc45298aa8f8ca 100644 (file)
--- a/modules/http2/h2_bucket_beam.c
+++ b/modules/http2/h2_bucket_beam.c
@@ -461,6 +461,12 @@ 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);
         }
@@ -620,6 +626,8 @@ void h2_beam_abort(h2_bucket_beam *beam)
             r_purge_sent(beam);
             h2_blist_cleanup(&beam->send_list);
             report_consumption(beam, 0);
+            ap_log_perror(APLOG_MARK, APLOG_WARNING, 0, beam->send_pool, 
+                          "h2_beam(%d-%s): aborted", beam->id, beam->tag);
         }
         if (beam->m_cond) {
             apr_thread_cond_broadcast(beam->m_cond);
@@ -799,6 +807,17 @@ static apr_status_t append_bucket(h2_bucket_beam *beam,
     return APR_SUCCESS;
 }
 
+void h2_beam_send_from(h2_bucket_beam *beam, apr_pool_t *p)
+{
+    h2_beam_lock bl;
+    /* Called from the red thread to add buckets to the beam */
+    if (enter_yellow(beam, &bl) == APR_SUCCESS) {
+        r_purge_sent(beam);
+        beam_set_send_pool(beam, p);
+        leave_yellow(beam, &bl);
+    }
+}
+
 apr_status_t h2_beam_send(h2_bucket_beam *beam, 
                           apr_bucket_brigade *red_brigade, 
                           apr_read_type_e block)
@@ -809,8 +828,10 @@ apr_status_t h2_beam_send(h2_bucket_beam *beam,
 
     /* Called from the red thread to add buckets to the beam */
     if (enter_yellow(beam, &bl) == APR_SUCCESS) {
+        ap_log_perror(APLOG_MARK, APLOG_WARNING, 0, beam->send_pool, 
+                      "h2_beam(%d-%s): send", beam->id, beam->tag);
         r_purge_sent(beam);
-        if (red_brigade) {
+        if (red_brigade && !beam->send_pool) {
             beam_set_send_pool(beam, red_brigade->p);
         }
         
@@ -850,6 +871,8 @@ apr_status_t h2_beam_receive(h2_bucket_beam *beam,
     /* Called from the green thread to take buckets from the beam */
     if (enter_yellow(beam, &bl) == APR_SUCCESS) {
 transfer:
+        ap_log_perror(APLOG_MARK, APLOG_WARNING, 0, beam->send_pool, 
+                      "h2_beam(%d-%s): receive", beam->id, beam->tag);
         if (beam->aborted) {
             if (beam->recv_buffer && !APR_BRIGADE_EMPTY(beam->recv_buffer)) {
                 apr_brigade_cleanup(beam->recv_buffer);

diff --git a/modules/http2/h2_bucket_beam.h b/modules/http2/h2_bucket_beam.h
index fc6772be3ffe56a1cd01d93d2242276b1d679260..39fd476c20bbb23f31c5bf7747882e0310ee08c7 100644 (file)
--- a/modules/http2/h2_bucket_beam.h
+++ b/modules/http2/h2_bucket_beam.h
@@ -254,6 +254,13 @@ apr_status_t h2_beam_send(h2_bucket_beam *beam,
                           apr_bucket_brigade *bb, 
                           apr_read_type_e block);
 
+/**
+ * Register the pool from which future buckets are send. This defines
+ * the lifetime of the buckets, e.g. the pool should not be cleared/destroyed
+ * until the data is no longer needed (or has been received).
+ */
+void h2_beam_send_from(h2_bucket_beam *beam, apr_pool_t *p);
+
 /**
  * Receive buckets from the beam into the given brigade. Will return APR_EOF
  * when reading past an EOS bucket. Reads can be blocking until data is 

diff --git a/modules/http2/h2_mplx.c b/modules/http2/h2_mplx.c
index dd9c563ac37968080362d41fab4cb5efbaf9feed..5823bd8e547cce0c3bf68c30eeb28ca02aabd80a 100644 (file)
--- a/modules/http2/h2_mplx.c
+++ b/modules/http2/h2_mplx.c
@@ -53,8 +53,8 @@ static void h2_beam_log(h2_bucket_beam *beam, int id, const char *msg,
         apr_size_t off = 0;
         
         off += apr_snprintf(buffer+off, H2_ALEN(buffer)-off, "cl=%d, ", beam->closed);
-        off += h2_util_bl_print(buffer+off, H2_ALEN(buffer)-off, "red", ", ", &beam->send_list);
-        off += h2_util_bb_print(buffer+off, H2_ALEN(buffer)-off, "green", ", ", beam->recv_buffer);
+        off += h2_util_bl_print(buffer+off, H2_ALEN(buffer)-off, "to_send", ", ", &beam->send_list);
+        off += h2_util_bb_print(buffer+off, H2_ALEN(buffer)-off, "recv_buffer", ", ", beam->recv_buffer);
         off += h2_util_bl_print(buffer+off, H2_ALEN(buffer)-off, "hold", ", ", &beam->hold_list);
         off += h2_util_bl_print(buffer+off, H2_ALEN(buffer)-off, "purge", "", &beam->purge_list);
 
@@ -703,9 +703,14 @@ static apr_status_t out_open(h2_mplx *m, int stream_id, h2_bucket_beam *beam)
         return APR_ECONNABORTED;
     }
     
-    ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, m->c,
-                  "h2_mplx(%s): out open", task->id);
-                      
+    if (APLOGctrace2(m->c)) {
+        h2_beam_log(beam, stream_id, "out_open", m->c, APLOG_TRACE2);
+    }
+    else {
+        ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, m->c,
+                      "h2_mplx(%s): out open", task->id);
+    }
+    
     h2_beam_on_consumed(stream->output, stream_output_consumed, task);
     h2_beam_on_produced(stream->output, output_produced, m);
     beamed_count = h2_beam_get_files_beamed(stream->output);

diff --git a/modules/http2/h2_proxy_session.c b/modules/http2/h2_proxy_session.c
index 25d1eb3d181a39ff09b166edd953a853455ef82f..1dd2a418ddf5a3dc8b4704cf2f91d96608d29fb8 100644 (file)
--- a/modules/http2/h2_proxy_session.c
+++ b/modules/http2/h2_proxy_session.c
@@ -344,7 +344,7 @@ static void h2_proxy_stream_end_headers_out(h2_proxy_stream *stream)
         
         /* If USE_CANONICAL_NAME_OFF was configured for the proxy virtual host,
          * then the server name returned by ap_get_server_name() is the
-         * origin server name (which does make too much sense with Via: headers)
+         * origin server name (which doesn't make sense with Via: headers)
          * so we use the proxy vhost's name instead.
          */
         if (server_name == stream->r->hostname) {

diff --git a/modules/http2/h2_stream.c b/modules/http2/h2_stream.c
index d31193909c77bb41fa69e3648db08c37508a3dae..815b6e313b348b093f5bd3156a28ff5742474b6b 100644 (file)
--- a/modules/http2/h2_stream.c
+++ b/modules/http2/h2_stream.c
@@ -203,6 +203,7 @@ h2_stream *h2_stream_open(int id, apr_pool_t *pool, h2_session *session,
     stream->can_be_cleaned = 1;
 
     h2_beam_create(&stream->input, pool, id, "input", H2_BEAM_OWNER_SEND, 0);
+    h2_beam_send_from(stream->input, stream->pool);
     h2_beam_create(&stream->output, pool, id, "output", H2_BEAM_OWNER_RECV, 0);
     
     set_state(stream, H2_STREAM_ST_OPEN);

diff --git a/modules/http2/h2_task.c b/modules/http2/h2_task.c
index 1419aab5516c141542b9a56870d9c6641db3a398..b47300d835df8f24e96af546b6caeb90c7adf248 100644 (file)
--- a/modules/http2/h2_task.c
+++ b/modules/http2/h2_task.c
@@ -64,6 +64,24 @@ static void H2_TASK_OUT_LOG(int lvl, h2_task *task, apr_bucket_brigade *bb,
     }
 }
 
+static void h2_beam_log(h2_bucket_beam *beam, int id, const char *msg, 
+                        conn_rec *c, int level)
+{
+    if (beam && APLOG_C_IS_LEVEL(c,level)) {
+        char buffer[2048];
+        apr_size_t off = 0;
+        
+        off += apr_snprintf(buffer+off, H2_ALEN(buffer)-off, "cl=%d, ", beam->closed);
+        off += h2_util_bl_print(buffer+off, H2_ALEN(buffer)-off, "red", ", ", &beam->send_list);
+        off += h2_util_bb_print(buffer+off, H2_ALEN(buffer)-off, "green", ", ", beam->recv_buffer);
+        off += h2_util_bl_print(buffer+off, H2_ALEN(buffer)-off, "hold", ", ", &beam->hold_list);
+        off += h2_util_bl_print(buffer+off, H2_ALEN(buffer)-off, "purge", "", &beam->purge_list);
+
+        ap_log_cerror(APLOG_MARK, level, 0, c, "beam(%ld-%d): %s %s", 
+                      c->id, id, msg, buffer);
+    }
+}
+
 /*******************************************************************************
  * task input handling
  ******************************************************************************/
@@ -97,9 +115,12 @@ static apr_status_t send_out(h2_task *task, apr_bucket_brigade* bb, int block)
 
     apr_brigade_length(bb, 0, &written);
     H2_TASK_OUT_LOG(APLOG_TRACE2, task, bb, "h2_task send_out");
+    h2_beam_log(task->output.beam, task->stream_id, "send_out(before)", task->c, APLOG_TRACE2);
     /* engines send unblocking */
     status = h2_beam_send(task->output.beam, bb, 
                           block? APR_BLOCK_READ : APR_NONBLOCK_READ);
+    h2_beam_log(task->output.beam, task->stream_id, "send_out(after)", task->c, APLOG_TRACE2);
+    
     if (APR_STATUS_IS_EAGAIN(status)) {
         apr_brigade_length(bb, 0, &left);
         written -= left;
@@ -508,9 +529,10 @@ h2_task *h2_task_create(conn_rec *c, int stream_id, const h2_request *req,
     task->input.beam  = input;
     task->output.beam = output;
     
+    h2_beam_send_from(output, task->pool);
     apr_thread_cond_create(&task->cond, pool);
-
     h2_ctx_create_for(c, task);
+    
     return task;
 }