nghttpx: Fix request re-submission bug in HTTP/2 backend

src/shrpx_downstream.cc

@@ -122,7 +122,7 @@ Downstream::Downstream(Upstream *upstream, int32_t stream_id, int32_t priority)
       request_connection_close_(false), request_header_key_prev_(false),
       request_http2_expect_body_(false), chunked_response_(false),
       response_connection_close_(false), response_header_key_prev_(false),
-      expect_final_response_(false) {
+      expect_final_response_(false), request_pending_(false) {
 
   ev_timer_init(&upstream_rtimer_, &upstream_rtimeoutcb, 0.,
                 get_config()->stream_read_timeout);
@@ -1064,4 +1064,14 @@ void Downstream::set_request_downstream_host(std::string host) {
   request_downstream_host_ = std::move(host);
 }
 
+void Downstream::set_request_pending(bool f) { request_pending_ = f; }
+
+bool Downstream::get_request_pending() const { return request_pending_; }
+
+bool Downstream::request_submission_ready() const {
+  return (request_state_ == Downstream::HEADER_COMPLETE ||
+          request_state_ == Downstream::MSG_COMPLETE) &&
+         request_pending_ && response_state_ == Downstream::INITIAL;
+}
+
 } // namespace shrpx

src/shrpx_downstream.h

@@ -183,6 +183,10 @@ public:
   void set_request_state(int state);
   int get_request_state() const;
   DefaultMemchunks *get_request_buf();
+  void set_request_pending(bool f);
+  bool get_request_pending() const;
+  // Returns true if request is ready to be submitted to downstream.
+  bool request_submission_ready() const;
   // downstream response API
   const Headers &get_response_headers() const;
   // Lower the response header field names and indexes response
@@ -387,6 +391,10 @@ private:
   bool response_connection_close_;
   bool response_header_key_prev_;
   bool expect_final_response_;
+  // true if downstream request is pending because backend connection
+  // has not been established or should be checked before use;
+  // currently used only with HTTP/2 connection.
+  bool request_pending_;
 };
 
 } // namespace shrpx

src/shrpx_http2_downstream_connection.cc

@@ -222,16 +222,19 @@ ssize_t http2_data_read_callback(nghttp2_session *session, int32_t stream_id,
 
 int Http2DownstreamConnection::push_request_headers() {
   int rv;
+  if (!downstream_) {
+    return 0;
+  }
   if (!http2session_->can_push_request()) {
     // The HTTP2 session to the backend has not been established or
     // connection is now being checked.  This function will be called
     // again just after it is established.
+    downstream_->set_request_pending(true);
     http2session_->start_checking_connection();
     return 0;
   }
-  if (!downstream_) {
-    return 0;
-  }
+
+  downstream_->set_request_pending(false);
 
   const char *authority = nullptr, *host = nullptr;
   if (!get_config()->no_host_rewrite && !get_config()->http2_proxy &&

src/shrpx_http2_session.cc

@@ -113,11 +113,12 @@ void writecb(struct ev_loop *loop, ev_io *w, int revents) {
   auto conn = static_cast<Connection *>(w->data);
   auto http2session = static_cast<Http2Session *>(conn->data);
   http2session->clear_write_request();
-  http2session->connection_alive();
   rv = http2session->do_write();
   if (rv != 0) {
     http2session->disconnect(http2session->should_hard_fail());
+    return;
   }
+  http2session->connection_alive();
 }
 } // namespace
 
@@ -1288,29 +1289,8 @@ int Http2Session::on_connect() {
 
   reset_connection_check_timer();
 
-  // submit pending request
-  for (auto dconn : dconns_) {
-    if (dconn->push_request_headers() == 0) {
-      auto downstream = dconn->get_downstream();
-      auto upstream = downstream->get_upstream();
-      upstream->resume_read(SHRPX_NO_BUFFER, downstream, 0);
-      continue;
-    }
+  submit_pending_requests();
 
-    if (LOG_ENABLED(INFO)) {
-      SSLOG(INFO, this) << "backend request failed";
-    }
-
-    auto downstream = dconn->get_downstream();
-
-    if (!downstream) {
-      continue;
-    }
-
-    auto upstream = downstream->get_upstream();
-
-    upstream->on_downstream_abort_request(downstream, 400);
-  }
   signal_write();
   return 0;
 }
@@ -1476,28 +1456,30 @@ void Http2Session::connection_alive() {
   SSLOG(INFO, this) << "Connection alive";
   connection_check_state_ = CONNECTION_CHECK_NONE;
 
-  // submit pending request
+  submit_pending_requests();
+}
+
+void Http2Session::submit_pending_requests() {
   for (auto dconn : dconns_) {
     auto downstream = dconn->get_downstream();
-    if (!downstream ||
-        (downstream->get_request_state() != Downstream::HEADER_COMPLETE &&
-         downstream->get_request_state() != Downstream::MSG_COMPLETE) ||
-        downstream->get_response_state() != Downstream::INITIAL) {
+
+    if (!downstream || !downstream->request_submission_ready()) {
       continue;
     }
 
     auto upstream = downstream->get_upstream();
 
-    if (dconn->push_request_headers() == 0) {
-      upstream->resume_read(SHRPX_NO_BUFFER, downstream, 0);
+    if (dconn->push_request_headers() != 0) {
+      if (LOG_ENABLED(INFO)) {
+        SSLOG(INFO, this) << "backend request failed";
+      }
+
+      upstream->on_downstream_abort_request(downstream, 400);
+
       continue;
     }
 
-    if (LOG_ENABLED(INFO)) {
-      SSLOG(INFO, this) << "backend request failed";
-    }
-
-    upstream->on_downstream_abort_request(downstream, 400);
+    upstream->resume_read(SHRPX_NO_BUFFER, downstream, 0);
   }
 }
 

src/shrpx_http2_session.h

@@ -141,6 +141,8 @@ public:
 
   bool should_hard_fail() const;
 
+  void submit_pending_requests();
+
   enum {
     // Disconnected
     DISCONNECTED,

src/shrpx_http2_upstream.cc

@@ -1476,9 +1476,7 @@ int Http2Upstream::on_downstream_reset(bool no_retry) {
 
   for (auto &ent : downstream_queue_.get_active_downstreams()) {
     auto downstream = ent.second.get();
-    if ((downstream->get_request_state() != Downstream::HEADER_COMPLETE &&
-         downstream->get_request_state() != Downstream::MSG_COMPLETE) ||
-        downstream->get_response_state() != Downstream::INITIAL) {
+    if (!downstream->request_submission_ready()) {
       rst_stream(downstream, NGHTTP2_INTERNAL_ERROR);
       downstream->pop_downstream_connection();
       continue;

src/shrpx_https_upstream.cc

@@ -839,9 +839,7 @@ void HttpsUpstream::on_handler_delete() {
 int HttpsUpstream::on_downstream_reset(bool no_retry) {
   int rv;
 
-  if ((downstream_->get_request_state() != Downstream::HEADER_COMPLETE &&
-       downstream_->get_request_state() != Downstream::MSG_COMPLETE) ||
-      downstream_->get_response_state() != Downstream::INITIAL) {
+  if (!downstream_->request_submission_ready()) {
     // Return error so that caller can delete handler
     return -1;
   }

src/shrpx_spdy_upstream.cc

@@ -1041,9 +1041,7 @@ int SpdyUpstream::on_downstream_reset(bool no_retry) {
 
   for (auto &ent : downstream_queue_.get_active_downstreams()) {
     auto downstream = ent.second.get();
-    if ((downstream->get_request_state() != Downstream::HEADER_COMPLETE &&
-         downstream->get_request_state() != Downstream::MSG_COMPLETE) ||
-        downstream->get_response_state() != Downstream::INITIAL) {
+    if (!downstream->request_submission_ready()) {
       rst_stream(downstream, SPDYLAY_INTERNAL_ERROR);
       downstream->pop_downstream_connection();
       continue;