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

@@ -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**<N>-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;
       }

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";
 

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();

src/shrpx_http2_session.cc

@@ -48,14 +48,33 @@ 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<Http2Session *>(w->data);
-  SSLOG(INFO, http2session) << "connection check required";
+
   ev_timer_stop(loop, w);
+
+  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
 
 namespace {
@@ -90,12 +109,12 @@ void readcb(struct ev_loop *loop, ev_io *w, int revents) {
   int rv;
   auto conn = static_cast<Connection *>(w->data);
   auto http2session = static_cast<Http2Session *>(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;
   }
 
+  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() {

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<Http2DownstreamConnection *> dconns_;
   std::set<StreamData *> streams_;