From: Stefan Eissing Date: Sat, 27 Aug 2016 10:39:24 +0000 (+0000) Subject: mod_http2: fix for stream buffer handling during shutdown X-Git-Tag: 2.5.0-alpha~1195 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=2e6fa9fedc6450b512c9df0109f88abe42d40060;p=apache 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 5285016ba6..4a4801092a 100644 --- 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 ] diff --git a/modules/http2/h2_bucket_beam.c b/modules/http2/h2_bucket_beam.c index 6a1e74d3e7..1338ba68b0 100644 --- 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 fcafb063dd..8ccd8a3aaa 100644 --- 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 c1de2699b3..e340c9a678 100644 --- 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 e6214088aa..1cb0a59f2f 100644 --- 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 9b4c017567..f457eb49ff 100644 --- 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 7edfae754a..4394c706f0 100644 --- 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 75f376cf0f..318943ae4a 100644 --- 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 76e8780d2a..f13366e26c 100644 --- 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 a21aaf9ec5..a495b8e2d6 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.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 */