From 01d7740751bb95689f3dcb0983a0aaec2a358398 Mon Sep 17 00:00:00 2001 From: Stefan Eissing Date: Tue, 5 Jan 2016 16:56:18 +0000 Subject: [PATCH] no async http2 read for now, fixed GOAWAY flush at end of session, scoreboard keepalive status shown git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1723125 13f79535-47bb-0310-9956-ffa450edef68 --- CHANGES | 16 ++-- modules/http2/h2_conn.c | 3 - modules/http2/h2_filter.c | 1 - modules/http2/h2_session.c | 156 ++++++++++++++++--------------------- modules/http2/h2_session.h | 1 - 5 files changed, 71 insertions(+), 106 deletions(-) diff --git a/CHANGES b/CHANGES index 40f88076fe..d22273f955 100644 --- a/CHANGES +++ b/CHANGES @@ -1,19 +1,15 @@ -*- coding: utf-8 -*- Changes with Apache 2.5.0 + *) mod_http2: async reading for http2 connections disabled for now. Fixed + flushing of last GOAWAY frame. Fixed calculation of last stream id + accepted as described in rfc7540. Reading in KEEPALIVE state now + correctly show in scoreboard. Fixed possible race in connection + shutdown after review by Ylavic. [Stefan Eissing] + *) mod_ssl: Avoid one TLS record (application data) fragmentation by including the last suitable bucket when coalescing. [Yann Ylavic] - *) mod_http2: reworked synching of session shutdown with worker threads. h2 - workers now stick to a session until no more reuqquest are tbd, keepalive - handling revisited, users report problems with connection close without - GOAWAY frames. [Stefan Eissing] - - *) mod_http2: Fixed several errors when connections are closed in the middle - of requests, changed H2KeepAliveTimeout defaults to be the same as H2Timeout - for synchronous MPMs and leave keepalive timeout to async MPMs default. - [Stefan Eissing] - *) mod_cache_socache: Fix a possible cached entity body corruption when it is received from an origin server in multiple batches and forwarded by mod_proxy. [Yann Ylavic] diff --git a/modules/http2/h2_conn.c b/modules/http2/h2_conn.c index abc8d8989c..49570c9798 100644 --- a/modules/http2/h2_conn.c +++ b/modules/http2/h2_conn.c @@ -167,9 +167,6 @@ apr_status_t h2_conn_setup(h2_ctx *ctx, conn_rec *c, request_rec *r) } h2_ctx_session_set(ctx, session); - - ap_update_child_status_from_conn(c->sbh, SERVER_BUSY_READ, c); - return APR_SUCCESS; } diff --git a/modules/http2/h2_filter.c b/modules/http2/h2_filter.c index 5ad3aed5d7..fd8e25ce46 100644 --- a/modules/http2/h2_filter.c +++ b/modules/http2/h2_filter.c @@ -133,7 +133,6 @@ apr_status_t h2_filter_core_input(ap_filter_t* f, apr_socket_timeout_set(cin->socket, t); } } - ap_update_child_status_from_conn(f->c->sbh, SERVER_BUSY_READ, f->c); status = ap_get_brigade(f->next, cin->bb, AP_MODE_READBYTES, block, readbytes); if (saved_timeout != UNSET) { diff --git a/modules/http2/h2_session.c b/modules/http2/h2_session.c index 4b483addea..0cc0fcb621 100644 --- a/modules/http2/h2_session.c +++ b/modules/http2/h2_session.c @@ -695,6 +695,46 @@ static void h2_session_destroy(h2_session *session) } } +static apr_status_t h2_session_shutdown(h2_session *session, int reason) +{ + apr_status_t status = APR_SUCCESS; + + AP_DEBUG_ASSERT(session); + session->aborted = 1; + if (session->state != H2_SESSION_ST_CLOSING + && session->state != H2_SESSION_ST_ABORTED) { + h2_mplx_abort(session->mplx); + if (session->server_goaway) { + /* already sent one */ + } + else if (!reason) { + nghttp2_submit_goaway(session->ngh2, NGHTTP2_FLAG_NONE, + h2_mplx_get_max_stream_started(session->mplx), + reason, NULL, 0); + status = nghttp2_session_send(session->ngh2); + session->server_goaway = 1; + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c, + "session(%ld): shutdown, no err", session->id); + } + else { + const char *err = nghttp2_strerror(reason); + nghttp2_submit_goaway(session->ngh2, NGHTTP2_FLAG_NONE, + h2_mplx_get_max_stream_started(session->mplx), + reason, (const uint8_t *)err, + strlen(err)); + status = nghttp2_session_send(session->ngh2); + session->server_goaway = 1; + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c, + "session(%ld): shutdown, err=%d '%s'", + session->id, reason, err); + } + + h2_conn_io_flush(&session->io); + session->state = H2_SESSION_ST_CLOSING; + } + return status; +} + static apr_status_t session_pool_cleanup(void *data) { h2_session *session = data; @@ -707,6 +747,22 @@ 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); + + AP_DEBUG_ASSERT(session->aborted || session->server_goaway); + if (!session->aborted && !session->server_goaway) { + /* 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 + * been processed and are not safe to retry. + * As clients with idle connection may only learn about a closed + * connection when sending the next request, this has the effect + * that at least this one request will fail. + */ + ap_log_cerror(APLOG_MARK, APLOG_WARNING, 0, session->c, + "session(%ld): connection disappeared without proper " + "goodbye, clients will be confused, should not happen", + session->id); + } /* keep us from destroying the pool, since that is already ongoing. */ session->pool = NULL; h2_session_destroy(session); @@ -889,44 +945,6 @@ void h2_session_eoc_callback(h2_session *session) h2_session_destroy(session); } -static apr_status_t h2_session_shutdown(h2_session *session, int reason) -{ - apr_status_t status = APR_SUCCESS; - - AP_DEBUG_ASSERT(session); - session->aborted = 1; - if (session->state != H2_SESSION_ST_CLOSING - && session->state != H2_SESSION_ST_ABORTED) { - h2_mplx_abort(session->mplx); - if (session->server_goaway) { - /* already sent one */ - } - else if (!reason) { - nghttp2_submit_goaway(session->ngh2, NGHTTP2_FLAG_NONE, - h2_mplx_get_max_stream_started(session->mplx), - reason, NULL, 0); - status = nghttp2_session_send(session->ngh2); - session->server_goaway = 1; - ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c, - "session(%ld): shutdown, no err", session->id); - } - else { - const char *err = nghttp2_strerror(reason); - nghttp2_submit_goaway(session->ngh2, NGHTTP2_FLAG_NONE, - h2_mplx_get_max_stream_started(session->mplx), - reason, (const uint8_t *)err, - strlen(err)); - status = nghttp2_session_send(session->ngh2); - session->server_goaway = 1; - ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c, - "session(%ld): shutdown, err=%d '%s'", - session->id, reason, err); - } - session->state = H2_SESSION_ST_CLOSING; - } - return status; -} - void h2_session_abort(h2_session *session, apr_status_t status) { AP_DEBUG_ASSERT(session); @@ -1716,7 +1734,7 @@ apr_status_t h2_session_process(h2_session *session, int async) { apr_status_t status = APR_SUCCESS; conn_rec *c = session->c; - int rv, have_written, have_read, remain_secs; + int rv, have_written, have_read; const char *reason = ""; ap_log_cerror( APLOG_MARK, APLOG_TRACE1, status, c, @@ -1740,6 +1758,7 @@ apr_status_t h2_session_process(h2_session *session, int async) session->server_goaway = 1; } + ap_update_child_status(c->sbh, SERVER_BUSY_READ, NULL); status = h2_session_start(session, &rv); ap_log_cerror(APLOG_MARK, APLOG_DEBUG, status, c, "h2_session(%ld): started on %s:%d", session->id, @@ -1755,13 +1774,14 @@ apr_status_t h2_session_process(h2_session *session, int async) break; case H2_SESSION_ST_IDLE_READ: - h2_filter_cin_timeout_set(session->cin, session->timeout_secs); - ap_update_child_status(c->sbh, SERVER_BUSY_READ, NULL); + h2_filter_cin_timeout_set(session->cin, session->keepalive_secs); + ap_update_child_status(c->sbh, SERVER_BUSY_KEEPALIVE, NULL); status = h2_session_read(session, 1, 10); if (APR_STATUS_IS_TIMEUP(status)) { ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, c, "h2_session(%ld): IDLE -> KEEPALIVE", session->id); - session->state = H2_SESSION_ST_KEEPALIVE; + h2_session_shutdown(session, 0); + goto out; } else if (status == APR_SUCCESS) { /* got something, go busy again */ @@ -1777,6 +1797,7 @@ apr_status_t h2_session_process(h2_session *session, int async) case H2_SESSION_ST_BUSY: if (nghttp2_session_want_read(session->ngh2)) { + ap_update_child_status(c->sbh, SERVER_BUSY_READ, NULL); h2_filter_cin_timeout_set(session->cin, session->timeout_secs); status = h2_session_read(session, 0, 10); if (status == APR_SUCCESS) { @@ -1879,6 +1900,7 @@ apr_status_t h2_session_process(h2_session *session, int async) } else if (status == APR_TIMEUP) { if (nghttp2_session_want_read(session->ngh2)) { + ap_update_child_status(c->sbh, SERVER_BUSY_READ, NULL); status = h2_session_read(session, 0, 1); if (status == APR_SUCCESS) { /* got something, go busy again */ @@ -1904,57 +1926,9 @@ apr_status_t h2_session_process(h2_session *session, int async) } break; - case H2_SESSION_ST_KEEPALIVE: - /* Our normal H2Timeout has passed and we are considering to - * extend that with the H2KeepAliveTimeout. */ - remain_secs = session->keepalive_secs - session->timeout_secs; - if (remain_secs <= 0) { - /* keepalive is <= normal timeout, close the session */ - reason = "keepalive expired"; - h2_session_shutdown(session, 0); - goto out; - } - session->c->keepalive = AP_CONN_KEEPALIVE; - ap_update_child_status_from_conn(c->sbh, SERVER_BUSY_KEEPALIVE, c); - - if ((apr_time_sec(session->s->keep_alive_timeout) >= remain_secs) - && async && session->c->cs - && !session->r) { - /* Async MPMs are able to handle keep-alive connections without - * blocking a thread. For this to happen, we need to return from - * processing, indicating the IO event we are waiting for, and - * may be called again if the event happens. - * TODO: this does not properly GOAWAY connections... - * TODO: This currently does not work on upgraded requests... - */ - ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, c, - "h2_session(%ld): async KEEPALIVE -> IDLE_READ", session->id); - session->state = H2_SESSION_ST_IDLE_READ; - session->c->cs->state = CONN_STATE_WRITE_COMPLETION; - reason = "async keepalive"; - status = APR_SUCCESS; - goto out; - } - ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, c, - "h2_session(%ld): KEEPALIVE read", session->id); - h2_filter_cin_timeout_set(session->cin, remain_secs); - status = h2_session_read(session, 1, 1); - if (APR_STATUS_IS_TIMEUP(status)) { - reason = "keepalive expired"; - h2_session_shutdown(session, 0); - goto out; - } - else if (status != APR_SUCCESS) { - reason = "keepalive error"; - goto out; - } - ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, c, - "h2_session(%ld): KEEPALIVE -> BUSY", session->id); - session->state = H2_SESSION_ST_BUSY; - break; - case H2_SESSION_ST_CLOSING: if (nghttp2_session_want_write(session->ngh2)) { + ap_update_child_status(c->sbh, SERVER_BUSY_READ, NULL); status = h2_session_send(session); if (status != APR_SUCCESS) { reason = "send error"; diff --git a/modules/http2/h2_session.h b/modules/http2/h2_session.h index 2dba10f720..b3b26b7b00 100644 --- a/modules/http2/h2_session.h +++ b/modules/http2/h2_session.h @@ -58,7 +58,6 @@ typedef enum { H2_SESSION_ST_IDLE_READ, /* nothing to write, expecting data inc */ H2_SESSION_ST_BUSY, /* read/write without stop */ H2_SESSION_ST_BUSY_WAIT, /* waiting for tasks reporting back */ - H2_SESSION_ST_KEEPALIVE, /* nothing to write, normal timeout passed */ H2_SESSION_ST_CLOSING, /* shuting down */ H2_SESSION_ST_ABORTED, /* client closed connection or sky fall */ } h2_session_state; -- 2.40.0