From: Stefan Eissing Date: Fri, 23 Dec 2016 11:34:32 +0000 (+0000) Subject: On the trunk: X-Git-Tag: 2.5.0-alpha~879 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=a1d69148c2cdf11a0ed0c4046f9c79f4ecdc686e;p=apache 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 92e1c4bdf3..36fbe9c2a3 100644 --- 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 9b9c3b3a2f..3d78a7937d 100644 --- 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 fc6772be3f..39fd476c20 100644 --- 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 dd9c563ac3..5823bd8e54 100644 --- 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 25d1eb3d18..1dd2a418dd 100644 --- 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 d31193909c..815b6e313b 100644 --- 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 1419aab551..b47300d835 100644 --- 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; }