From: Stefan Eissing Date: Wed, 24 Aug 2016 13:30:11 +0000 (+0000) Subject: mod_http2: graceful handling of open streams during graceful shutdown X-Git-Tag: 2.5.0-alpha~1213 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0f69ba2dc8ed7a2d47cca16a4986286060c9f903;p=thirdparty%2Fapache%2Fhttpd.git mod_http2: graceful handling of open streams during graceful shutdown git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1757524 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/CHANGES b/CHANGES index 0a681c45fb3..b646303dad4 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,9 @@ -*- coding: utf-8 -*- Changes with Apache 2.5.0 + *) mod_http2: handling graceful shutdown gracefully, e.g. handling existing + streams to the end. [Stefan Eissing] + *) mod_cache: Use the actual URI path and query-string for identifying the cached entity (key), such that rewrites are taken into account when running afterwards (CacheQuickHandler off). PR 21935. [Yann Ylavic] diff --git a/modules/http2/h2.h b/modules/http2/h2.h index ab24947daaa..0492fe35b6e 100644 --- a/modules/http2/h2.h +++ b/modules/http2/h2.h @@ -95,8 +95,6 @@ typedef enum { H2_SESSION_ST_IDLE, /* nothing to write, expecting data inc */ H2_SESSION_ST_BUSY, /* read/write without stop */ H2_SESSION_ST_WAIT, /* waiting for tasks reporting back */ - H2_SESSION_ST_LOCAL_SHUTDOWN, /* we announced GOAWAY */ - H2_SESSION_ST_REMOTE_SHUTDOWN, /* client announced GOAWAY */ } h2_session_state; typedef struct h2_session_props { @@ -106,6 +104,7 @@ typedef struct h2_session_props { apr_uint32_t emitted_max; /* the highest local stream id sent */ apr_uint32_t error; /* the last session error encountered */ unsigned int accepting : 1; /* if the session is accepting new streams */ + unsigned int shutdown : 1; /* if the final GOAWAY has been sent */ } h2_session_props; diff --git a/modules/http2/h2_filter.c b/modules/http2/h2_filter.c index 68082a6ac3e..4a1b33df751 100644 --- a/modules/http2/h2_filter.c +++ b/modules/http2/h2_filter.c @@ -367,9 +367,7 @@ static apr_status_t h2_status_stream_filter(h2_stream *stream) add_peer_settings(bb, s, 0); bbout(bb, " \"connFlowIn\": %d,\n", connFlowIn); bbout(bb, " \"connFlowOut\": %d,\n", connFlowOut); - bbout(bb, " \"sentGoAway\": %d,\n", - (s->state == H2_SESSION_ST_LOCAL_SHUTDOWN - || s->state == H2_SESSION_ST_DONE)); + bbout(bb, " \"sentGoAway\": %d,\n", s->local.shutdown); add_streams(bb, s, 0); diff --git a/modules/http2/h2_session.c b/modules/http2/h2_session.c index 14be79de357..7874bfe2503 100644 --- a/modules/http2/h2_session.c +++ b/modules/http2/h2_session.c @@ -75,7 +75,6 @@ static apr_status_t h2_session_receive(void *ctx, const char *data, apr_size_t len, apr_size_t *readlen); -static int is_accepting_streams(h2_session *session); static void dispatch_event(h2_session *session, h2_session_event_t ev, int err, const char *msg); @@ -285,11 +284,6 @@ static int on_data_chunk_recv_cb(nghttp2_session *ngh2, uint8_t flags, int rv; (void)flags; - if (!is_accepting_streams(session)) { - /* ignore */ - return 0; - } - stream = get_stream(session, stream_id); if (!stream) { ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c, APLOGNO(03064) @@ -398,11 +392,6 @@ static int on_header_cb(nghttp2_session *ngh2, const nghttp2_frame *frame, apr_status_t status; (void)flags; - if (!is_accepting_streams(session)) { - /* just ignore */ - return 0; - } - stream = get_stream(session, frame->hd.stream_id); if (!stream) { ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, session->c, @@ -519,9 +508,16 @@ static int on_frame_recv_cb(nghttp2_session *ng2s, } break; case NGHTTP2_GOAWAY: - session->remote.accepted_max = frame->goaway.last_stream_id; - session->remote.error = frame->goaway.error_code; - dispatch_event(session, H2_SESSION_EV_REMOTE_GOAWAY, 0, NULL); + if (frame->goaway.error_code == 0 + && frame->goaway.last_stream_id == ((1u << 31) - 1)) { + /* shutdown notice. Should not come from a client... */ + session->remote.accepting = 0; + } + else { + session->remote.accepted_max = frame->goaway.last_stream_id; + dispatch_event(session, H2_SESSION_EV_REMOTE_GOAWAY, + frame->goaway.error_code, NULL); + } break; default: if (APLOGctrace2(session->c)) { @@ -715,12 +711,35 @@ static void h2_session_destroy(h2_session *session) } } +static apr_status_t h2_session_shutdown_notice(h2_session *session) +{ + apr_status_t status; + + AP_DEBUG_ASSERT(session); + if (!session->local.accepting) { + return APR_SUCCESS; + } + + nghttp2_submit_shutdown_notice(session->ngh2); + session->local.accepting = 0; + status = nghttp2_session_send(session->ngh2); + if (status == APR_SUCCESS) { + status = h2_conn_io_flush(&session->io); + } + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c, APLOGNO() + "session(%ld): sent shutdown notice", session->id); + return status; +} + static apr_status_t h2_session_shutdown(h2_session *session, int error, const char *msg, int force_close) { apr_status_t status = APR_SUCCESS; AP_DEBUG_ASSERT(session); + if (session->local.shutdown) { + return APR_SUCCESS; + } if (!msg && error) { msg = nghttp2_strerror(error); } @@ -744,6 +763,8 @@ static apr_status_t h2_session_shutdown(h2_session *session, int error, nghttp2_submit_goaway(session->ngh2, NGHTTP2_FLAG_NONE, session->local.accepted_max, error, (uint8_t*)msg, msg? strlen(msg):0); + session->local.accepting = 0; + session->local.shutdown = 1; status = nghttp2_session_send(session->ngh2); if (status == APR_SUCCESS) { status = h2_conn_io_flush(&session->io); @@ -773,8 +794,7 @@ static apr_status_t session_pool_cleanup(void *data) ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, session->c, "session(%ld): pool_cleanup", session->id); - if (session->state != H2_SESSION_ST_DONE - && session->state != H2_SESSION_ST_LOCAL_SHUTDOWN) { + if (session->state != H2_SESSION_ST_DONE) { /* 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 @@ -1596,9 +1616,6 @@ static apr_status_t h2_session_read(h2_session *session, int block) * status. */ return rstatus; } - if (!is_accepting_streams(session)) { - break; - } if ((session->io.bytes_read - read_start) > (64*1024)) { /* read enough in one go, give write a chance */ ap_log_cerror(APLOG_MARK, APLOG_TRACE2, status, c, @@ -1649,8 +1666,6 @@ static const char *StateNames[] = { "IDLE", /* H2_SESSION_ST_IDLE */ "BUSY", /* H2_SESSION_ST_BUSY */ "WAIT", /* H2_SESSION_ST_WAIT */ - "LSHUTDOWN", /* H2_SESSION_ST_LOCAL_SHUTDOWN */ - "RSHUTDOWN", /* H2_SESSION_ST_REMOTE_SHUTDOWN */ }; static const char *state_name(h2_session_state state) @@ -1661,18 +1676,6 @@ static const char *state_name(h2_session_state state) return StateNames[state]; } -static int is_accepting_streams(h2_session *session) -{ - switch (session->state) { - case H2_SESSION_ST_IDLE: - case H2_SESSION_ST_BUSY: - case H2_SESSION_ST_WAIT: - return 1; - default: - return 0; - } -} - static void update_child_status(h2_session *session, int status, const char *msg) { /* Assume that we also change code/msg when something really happened and @@ -1709,12 +1712,6 @@ static void transit(h2_session *session, const char *action, h2_session_state ns SERVER_BUSY_KEEPALIVE : SERVER_BUSY_READ), "idle"); break; - case H2_SESSION_ST_REMOTE_SHUTDOWN: - update_child_status(session, SERVER_CLOSING, "remote goaway"); - break; - case H2_SESSION_ST_LOCAL_SHUTDOWN: - update_child_status(session, SERVER_CLOSING, "local goaway"); - break; case H2_SESSION_ST_DONE: update_child_status(session, SERVER_CLOSING, "done"); break; @@ -1739,39 +1736,22 @@ 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) { - session->local.accepting = 0; cleanup_streams(session); - switch (session->state) { - case H2_SESSION_ST_LOCAL_SHUTDOWN: - /* already did that? */ - break; - case H2_SESSION_ST_IDLE: - case H2_SESSION_ST_REMOTE_SHUTDOWN: - /* all done */ - transit(session, "local goaway", H2_SESSION_ST_DONE); - break; - default: - transit(session, "local goaway", H2_SESSION_ST_LOCAL_SHUTDOWN); - break; + if (!session->remote.shutdown) { + update_child_status(session, SERVER_CLOSING, "local goaway"); } + transit(session, "local goaway", H2_SESSION_ST_DONE); } static void h2_session_ev_remote_goaway(h2_session *session, int arg, const char *msg) { - session->remote.accepting = 0; - cleanup_streams(session); - switch (session->state) { - case H2_SESSION_ST_REMOTE_SHUTDOWN: - /* already received that? */ - break; - case H2_SESSION_ST_IDLE: - case H2_SESSION_ST_LOCAL_SHUTDOWN: - /* all done */ - transit(session, "remote goaway", H2_SESSION_ST_DONE); - break; - default: - transit(session, "remote goaway", H2_SESSION_ST_REMOTE_SHUTDOWN); - break; + if (!session->remote.shutdown) { + session->remote.error = arg; + session->remote.accepting = 0; + session->remote.shutdown = 1; + cleanup_streams(session); + update_child_status(session, SERVER_CLOSING, "remote goaway"); + transit(session, "remote goaway", H2_SESSION_ST_DONE); } } @@ -1780,7 +1760,6 @@ static void h2_session_ev_conn_error(h2_session *session, int arg, const char *m switch (session->state) { case H2_SESSION_ST_INIT: case H2_SESSION_ST_DONE: - case H2_SESSION_ST_LOCAL_SHUTDOWN: /* just leave */ transit(session, "conn error", H2_SESSION_ST_DONE); break; @@ -1795,31 +1774,18 @@ static void h2_session_ev_conn_error(h2_session *session, int arg, const char *m static void h2_session_ev_proto_error(h2_session *session, int arg, const char *msg) { - switch (session->state) { - case H2_SESSION_ST_DONE: - case H2_SESSION_ST_LOCAL_SHUTDOWN: - /* just leave */ - transit(session, "proto error", H2_SESSION_ST_DONE); - break; - - default: - ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c, APLOGNO(03402) - "h2_session(%ld): proto error -> shutdown", session->id); - h2_session_shutdown(session, arg, msg, 0); - break; + if (!session->local.shutdown) { + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c, APLOGNO(03402) + "h2_session(%ld): proto error -> shutdown", session->id); + h2_session_shutdown(session, arg, msg, 0); } } static void h2_session_ev_conn_timeout(h2_session *session, int arg, const char *msg) { - switch (session->state) { - case H2_SESSION_ST_LOCAL_SHUTDOWN: - transit(session, "conn timeout", H2_SESSION_ST_DONE); - break; - default: - h2_session_shutdown(session, arg, msg, 1); - transit(session, "conn timeout", H2_SESSION_ST_DONE); - break; + transit(session, msg, H2_SESSION_ST_DONE); + if (!session->local.shutdown) { + h2_session_shutdown(session, arg, msg, 1); } } @@ -1827,8 +1793,6 @@ static void h2_session_ev_no_io(h2_session *session, int arg, const char *msg) { switch (session->state) { case H2_SESSION_ST_BUSY: - case H2_SESSION_ST_LOCAL_SHUTDOWN: - case H2_SESSION_ST_REMOTE_SHUTDOWN: /* Nothing to READ, nothing to WRITE on the master connection. * Possible causes: * - we wait for the client to send us sth @@ -1838,6 +1802,7 @@ static void h2_session_ev_no_io(h2_session *session, int arg, const char *msg) ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, session->c, "h2_session(%ld): NO_IO event, %d streams open", session->id, session->open_streams); + h2_conn_io_flush(&session->io); if (session->open_streams > 0) { if (has_unsubmitted_streams(session) || has_suspended_streams(session)) { @@ -1861,7 +1826,7 @@ static void h2_session_ev_no_io(h2_session *session, int arg, const char *msg) } } } - else if (is_accepting_streams(session)) { + else if (session->local.accepting) { /* When we have no streams, but accept new, switch to idle */ apr_time_t now = apr_time_now(); transit(session, "no io (keepalive)", H2_SESSION_ST_IDLE); @@ -1924,26 +1889,17 @@ static void h2_session_ev_mpm_stopping(h2_session *session, int arg, const char { switch (session->state) { case H2_SESSION_ST_DONE: - case H2_SESSION_ST_LOCAL_SHUTDOWN: /* nop */ break; default: - h2_session_shutdown(session, arg, msg, 0); + h2_session_shutdown_notice(session); break; } } static void h2_session_ev_pre_close(h2_session *session, int arg, const char *msg) { - switch (session->state) { - case H2_SESSION_ST_DONE: - case H2_SESSION_ST_LOCAL_SHUTDOWN: - /* nop */ - break; - default: - h2_session_shutdown(session, arg, msg, 1); - break; - } + h2_session_shutdown(session, arg, msg, 1); } static void h2_session_ev_stream_open(h2_session *session, int arg, const char *msg) @@ -2053,14 +2009,14 @@ apr_status_t h2_session_process(h2_session *session, int async) c->cs->state = CONN_STATE_WRITE_COMPLETION; } - while (1) { + while (session->state != H2_SESSION_ST_DONE) { trace = APLOGctrace3(c); session->have_read = session->have_written = 0; - if (!ap_mpm_query(AP_MPMQ_MPM_STATE, &mpm_state)) { + if (session->local.accepting + && !ap_mpm_query(AP_MPMQ_MPM_STATE, &mpm_state)) { if (mpm_state == AP_MPMQ_STOPPING) { dispatch_event(session, H2_SESSION_EV_MPM_STOPPING, 0, NULL); - break; } } @@ -2189,8 +2145,6 @@ apr_status_t h2_session_process(h2_session *session, int async) break; case H2_SESSION_ST_BUSY: - case H2_SESSION_ST_LOCAL_SHUTDOWN: - case H2_SESSION_ST_REMOTE_SHUTDOWN: if (nghttp2_session_want_read(session->ngh2)) { ap_update_child_status(session->c->sbh, SERVER_BUSY_READ, NULL); h2_filter_cin_timeout_set(session->cin, session->s->timeout); @@ -2273,7 +2227,7 @@ apr_status_t h2_session_process(h2_session *session, int async) else if (APR_STATUS_IS_TIMEUP(status)) { /* go back to checking all inputs again */ transit(session, "wait cycle", session->local.accepting? - H2_SESSION_ST_BUSY : H2_SESSION_ST_LOCAL_SHUTDOWN); + H2_SESSION_ST_BUSY : H2_SESSION_ST_DONE); } else if (APR_STATUS_IS_ECONNRESET(status) || APR_STATUS_IS_ECONNABORTED(status)) { @@ -2289,10 +2243,6 @@ apr_status_t h2_session_process(h2_session *session, int async) } break; - case H2_SESSION_ST_DONE: - status = APR_EOF; - goto out; - default: ap_log_cerror(APLOG_MARK, APLOG_ERR, APR_EGENERAL, c, APLOGNO(03080) @@ -2322,11 +2272,12 @@ out: && (APR_STATUS_IS_EOF(status) || APR_STATUS_IS_ECONNRESET(status) || APR_STATUS_IS_ECONNABORTED(status))) { - dispatch_event(session, H2_SESSION_EV_CONN_ERROR, 0, NULL); - } + dispatch_event(session, H2_SESSION_EV_CONN_ERROR, 0, NULL); + } - status = (session->state == H2_SESSION_ST_DONE)? APR_EOF : APR_SUCCESS; + status = APR_SUCCESS; if (session->state == H2_SESSION_ST_DONE) { + status = APR_EOF; if (!session->eoc_written) { session->eoc_written = 1; h2_conn_io_write_eoc(&session->io, session); @@ -2340,6 +2291,6 @@ apr_status_t h2_session_pre_close(h2_session *session, int async) { ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, session->c, "h2_session(%ld): pre_close", session->id); - dispatch_event(session, H2_SESSION_EV_PRE_CLOSE, 0, "timeout"); + dispatch_event(session, H2_SESSION_EV_PRE_CLOSE, 0, NULL); return APR_SUCCESS; }