From 41e266181eeee8ef0aeb7a51fcf076f0addffe97 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Mon, 9 Mar 2015 21:22:31 +0900 Subject: [PATCH] nghttpx: Attempt to improve HTTP/2 backend connection check It turns out that writing successfully to network is not enough. After apparently successful network write, read fails and then we first know network has been lost (at least my android mobile network). In this change, we say connection check is successful only when successful read. We already send PING in this case, so we just wait PING ACK with short timeout. If timeout has expired, drop connection. Since waiting for PING ACK could degrade performance for fast reliably connected network, we decided to disable connection check by default. Use --backend-http2-connection-check to enable it. --- src/shrpx.cc | 10 ++++++ src/shrpx_config.cc | 8 +++++ src/shrpx_config.h | 2 ++ src/shrpx_http2_session.cc | 63 +++++++++++++++++++++++++++++++------- src/shrpx_http2_session.h | 12 ++++++-- 5 files changed, 81 insertions(+), 14 deletions(-) diff --git a/src/shrpx.cc b/src/shrpx.cc index e7f9df9f..09438c07 100644 --- a/src/shrpx.cc +++ b/src/shrpx.cc @@ -917,6 +917,7 @@ void fill_default_config() { mod_config()->downstream_response_buffer_size = 16 * 1024; mod_config()->no_server_push = false; mod_config()->host_unix = false; + mod_config()->http2_downstream_connchk = false; } } // namespace @@ -1217,6 +1218,10 @@ HTTP/2 and SPDY: connection to 2**-1. Default: )" << get_config()->http2_downstream_connection_window_bits << R"( + --backend-http2-connection-check + Check HTTP/2 backend connection is alive before + forwarding request. This option is useful in unstable + network environment (e.g. mobile). --backend-no-tls Disable SSL/TLS on backend connections. --http2-no-cookie-crumbling @@ -1489,6 +1494,7 @@ int main(int argc, char **argv) { {"backend-request-buffer", required_argument, &flag, 72}, {"no-host-rewrite", no_argument, &flag, 73}, {"no-server-push", no_argument, &flag, 74}, + {"backend-http2-connection-check", no_argument, &flag, 75}, {nullptr, 0, nullptr, 0}}; int option_index = 0; @@ -1826,6 +1832,10 @@ int main(int argc, char **argv) { // --no-server-push cmdcfgs.emplace_back(SHRPX_OPT_NO_SERVER_PUSH, "yes"); break; + case 75: + // --backend-http2-connection-check + cmdcfgs.emplace_back(SHRPX_OPT_BACKEND_HTTP2_CONNECTION_CHECK, "yes"); + break; default: break; } diff --git a/src/shrpx_config.cc b/src/shrpx_config.cc index 56fa337d..9d9961cd 100644 --- a/src/shrpx_config.cc +++ b/src/shrpx_config.cc @@ -146,6 +146,8 @@ const char SHRPX_OPT_TLS_CTX_PER_WORKER[] = "tls-ctx-per-worker"; const char SHRPX_OPT_BACKEND_REQUEST_BUFFER[] = "backend-request-buffer"; const char SHRPX_OPT_BACKEND_RESPONSE_BUFFER[] = "backend-response-buffer"; const char SHRPX_OPT_NO_SERVER_PUSH[] = "no-server-push"; +const char SHRPX_OPT_BACKEND_HTTP2_CONNECTION_CHECK[] = + "backend-http2-connection-check"; namespace { Config *config = nullptr; @@ -1193,6 +1195,12 @@ int parse_config(const char *opt, const char *optarg) { return 0; } + if (util::strieq(opt, SHRPX_OPT_BACKEND_HTTP2_CONNECTION_CHECK)) { + mod_config()->http2_downstream_connchk = util::strieq(optarg, "yes"); + + return 0; + } + if (util::strieq(opt, "conf")) { LOG(WARN) << "conf: ignored"; diff --git a/src/shrpx_config.h b/src/shrpx_config.h index b64755f5..d4cec158 100644 --- a/src/shrpx_config.h +++ b/src/shrpx_config.h @@ -136,6 +136,7 @@ extern const char SHRPX_OPT_TLS_CTX_PER_WORKER[]; extern const char SHRPX_OPT_BACKEND_REQUEST_BUFFER[]; extern const char SHRPX_OPT_BACKEND_RESPONSE_BUFFER[]; extern const char SHRPX_OPT_NO_SERVER_PUSH[]; +extern const char SHRPX_OPT_BACKEND_HTTP2_CONNECTION_CHECK[]; union sockaddr_union { sockaddr_storage storage; @@ -322,6 +323,7 @@ struct Config { bool no_server_push; // true if host contains UNIX domain socket path bool host_unix; + bool http2_downstream_connchk; }; const Config *get_config(); diff --git a/src/shrpx_http2_session.cc b/src/shrpx_http2_session.cc index 92e668dd..4b3bbf38 100644 --- a/src/shrpx_http2_session.cc +++ b/src/shrpx_http2_session.cc @@ -48,13 +48,32 @@ using namespace nghttp2; namespace shrpx { +namespace { +const ev_tstamp CONNCHK_TIMEOUT = 5.; +const ev_tstamp CONNCHK_PING_TIMEOUT = 1.; +} // namespace + namespace { void connchk_timeout_cb(struct ev_loop *loop, ev_timer *w, int revents) { auto http2session = static_cast(w->data); - SSLOG(INFO, http2session) << "connection check required"; + ev_timer_stop(loop, w); - http2session->set_connection_check_state( - Http2Session::CONNECTION_CHECK_REQUIRED); + + switch (http2session->get_connection_check_state()) { + case Http2Session::CONNECTION_CHECK_STARTED: + // ping timeout; disconnect + if (LOG_ENABLED(INFO)) { + SSLOG(INFO, http2session) << "ping timeout"; + } + http2session->disconnect(); + return; + default: + if (LOG_ENABLED(INFO)) { + SSLOG(INFO, http2session) << "connection check required"; + } + http2session->set_connection_check_state( + Http2Session::CONNECTION_CHECK_REQUIRED); + } } } // namespace @@ -90,12 +109,12 @@ void readcb(struct ev_loop *loop, ev_io *w, int revents) { int rv; auto conn = static_cast(w->data); auto http2session = static_cast(conn->data); - http2session->connection_alive(); rv = http2session->do_read(); if (rv != 0) { http2session->disconnect(http2session->should_hard_fail()); return; } + http2session->connection_alive(); if (ev_is_active(http2session->get_wev())) { rv = http2session->do_write(); if (rv != 0) { @@ -116,7 +135,6 @@ void writecb(struct ev_loop *loop, ev_io *w, int revents) { http2session->disconnect(http2session->should_hard_fail()); return; } - http2session->connection_alive(); } } // namespace @@ -131,8 +149,9 @@ Http2Session::Http2Session(struct ev_loop *loop, SSL_CTX *ssl_ctx) read_ = write_ = &Http2Session::noop; on_read_ = on_write_ = &Http2Session::noop; - // We will resuse this many times, so use repeat timeout value. - ev_timer_init(&connchk_timer_, connchk_timeout_cb, 0., 5.); + // We will resuse this many times, so use repeat timeout value. The + // timeout value is set later. + ev_timer_init(&connchk_timer_, connchk_timeout_cb, 0., 0.); connchk_timer_.data = this; @@ -953,6 +972,14 @@ int on_frame_recv_callback(nghttp2_session *session, const nghttp2_frame *frame, } http2session->stop_settings_timer(); return 0; + case NGHTTP2_PING: + if (frame->hd.flags & NGHTTP2_FLAG_ACK) { + if (LOG_ENABLED(INFO)) { + LOG(INFO) << "PING ACK received"; + } + http2session->connection_alive(); + } + return 0; case NGHTTP2_PUSH_PROMISE: if (LOG_ENABLED(INFO)) { SSLOG(INFO, http2session) @@ -1234,7 +1261,7 @@ int Http2Session::on_connect() { return 0; } - reset_connection_check_timer(); + reset_connection_check_timer(CONNCHK_TIMEOUT); submit_pending_requests(); @@ -1397,21 +1424,31 @@ void Http2Session::start_checking_connection() { // ping frame to see whether connection is alive. nghttp2_submit_ping(session_, NGHTTP2_FLAG_NONE, NULL); + // set ping timeout and start timer again + reset_connection_check_timer(CONNCHK_PING_TIMEOUT); + signal_write(); } -void Http2Session::reset_connection_check_timer() { +void Http2Session::reset_connection_check_timer(ev_tstamp t) { + if (!get_config()->http2_downstream_connchk) { + return; + } + connchk_timer_.repeat = t; ev_timer_again(conn_.loop, &connchk_timer_); } void Http2Session::connection_alive() { - reset_connection_check_timer(); + reset_connection_check_timer(CONNCHK_TIMEOUT); if (connection_check_state_ == CONNECTION_CHECK_NONE) { return; } - SSLOG(INFO, this) << "Connection alive"; + if (LOG_ENABLED(INFO)) { + SSLOG(INFO, this) << "Connection alive"; + } + connection_check_state_ = CONNECTION_CHECK_NONE; submit_pending_requests(); @@ -1445,6 +1482,10 @@ void Http2Session::set_connection_check_state(int state) { connection_check_state_ = state; } +int Http2Session::get_connection_check_state() const { + return connection_check_state_; +} + int Http2Session::noop() { return 0; } int Http2Session::connected() { diff --git a/src/shrpx_http2_session.h b/src/shrpx_http2_session.h index a71a90ba..88918027 100644 --- a/src/shrpx_http2_session.h +++ b/src/shrpx_http2_session.h @@ -128,14 +128,16 @@ public: // Initiates the connection checking if downstream connection has // been established and connection checking is required. void start_checking_connection(); - // Resets connection check timer. After timeout, we require - // connection checking. - void reset_connection_check_timer(); + // Resets connection check timer to timeout |t|. After timeout, we + // require connection checking. If connection checking is already + // enabled, this timeout is for PING ACK timeout. + void reset_connection_check_timer(ev_tstamp t); // Signals that connection is alive. Internally // reset_connection_check_timer() is called. void connection_alive(); // Change connection check state. void set_connection_check_state(int state); + int get_connection_check_state() const; bool should_hard_fail() const; @@ -175,6 +177,10 @@ public: private: Connection conn_; ev_timer settings_timer_; + // This timer has 2 purpose: when it first timeout, set + // connection_check_state_ = CONNECTION_CHECK_REQUIRED. After + // connection check has started, this timer is started again and + // traps PING ACK timeout. ev_timer connchk_timer_; std::set dconns_; std::set streams_;