nghttpx: Handle h2 backend error per Downstream

Previously we wrongly handles stream per connection when h2 backend
failed or closed.  If upstream is h2 or spdy, streams which are not
associated to the failed h2 backend are also handled, which is
unnecessary.

src/shrpx_http2_session.cc

@@ -264,25 +264,28 @@ int Http2Session::disconnect(bool hard) {
   state_ = DISCONNECTED;
 
   // When deleting Http2DownstreamConnection, it calls this object's
-  // remove_downstream_connection(). The multiple
+  // remove_downstream_connection().  The multiple
   // Http2DownstreamConnection objects belong to the same
-  // ClientHandler object.  So first dump ClientHandler objects.
+  // ClientHandler object if upstream is h2 or SPDY.  So be careful
+  // when you delete ClientHandler here.
   //
   // We allow creating new pending Http2DownstreamConnection with this
   // object.  Upstream::on_downstream_reset() may add
-  // Http2DownstreamConnection.
+  // Http2DownstreamConnection to another Http2Session.
 
-  std::set<ClientHandler *> handlers;
-  for (auto dc = dconns_.head; dc; dc = dc->dlnext) {
-    if (!dc->get_client_handler()) {
-      continue;
-    }
-    handlers.insert(dc->get_client_handler());
-  }
-  for (auto h : handlers) {
-    if (h->get_upstream()->on_downstream_reset(hard) != 0) {
-      delete h;
+  for (auto dc = dconns_.head; dc;) {
+    auto next = dc->dlnext;
+    auto downstream = dc->get_downstream();
+    auto upstream = downstream->get_upstream();
+
+    // Failure is allowed only for HTTP/1 upstream where upstream is
+    // not shared by multiple Downstreams.
+    if (upstream->on_downstream_reset(downstream, hard) != 0) {
+      delete upstream->get_client_handler();
     }
+
+    // dc was deleted
+    dc = next;
   }
 
   auto streams = std::move(streams_);

src/shrpx_http2_upstream.cc

@@ -1812,64 +1812,66 @@ void Http2Upstream::on_handler_delete() {
   }
 }
 
-int Http2Upstream::on_downstream_reset(bool no_retry) {
+int Http2Upstream::on_downstream_reset(Downstream *downstream, bool no_retry) {
   int rv;
 
-  for (auto downstream = downstream_queue_.get_downstreams(); downstream;
-       downstream = downstream->dlnext) {
-    if (downstream->get_dispatch_state() != Downstream::DISPATCH_ACTIVE) {
-      // This is error condition when we failed push_request_headers()
-      // in initiate_downstream().  Otherwise, we have
-      // Downstream::DISPATCH_ACTIVE state, or we did not set
-      // DownstreamConnection.
-      downstream->pop_downstream_connection();
-      continue;
-    }
-
-    if (!downstream->request_submission_ready()) {
-      // pushed stream is handled here
-      rst_stream(downstream, NGHTTP2_INTERNAL_ERROR);
-      downstream->pop_downstream_connection();
-      continue;
-    }
-
+  if (downstream->get_dispatch_state() != Downstream::DISPATCH_ACTIVE) {
+    // This is error condition when we failed push_request_headers()
+    // in initiate_downstream().  Otherwise, we have
+    // Downstream::DISPATCH_ACTIVE state, or we did not set
+    // DownstreamConnection.
     downstream->pop_downstream_connection();
+    handler_->signal_write();
 
-    downstream->add_retry();
-
-    std::unique_ptr<DownstreamConnection> dconn;
-
-    if (no_retry || downstream->no_more_retry()) {
-      goto fail;
-    }
-
-    // downstream connection is clean; we can retry with new
-    // downstream connection.
-
-    dconn = handler_->get_downstream_connection(downstream);
-    if (!dconn) {
-      goto fail;
-    }
-
-    rv = downstream->attach_downstream_connection(std::move(dconn));
-    if (rv != 0) {
-      goto fail;
-    }
-
-    rv = downstream->push_request_headers();
-    if (rv != 0) {
-      goto fail;
-    }
-
-    continue;
-
-  fail:
-    if (on_downstream_abort_request(downstream, 503) != 0) {
-      return -1;
-    }
-    downstream->pop_downstream_connection();
+    return 0;
   }
 
+  if (!downstream->request_submission_ready()) {
+    // pushed stream is handled here
+    rst_stream(downstream, NGHTTP2_INTERNAL_ERROR);
+    downstream->pop_downstream_connection();
+
+    handler_->signal_write();
+
+    return 0;
+  }
+
+  downstream->pop_downstream_connection();
+
+  downstream->add_retry();
+
+  std::unique_ptr<DownstreamConnection> dconn;
+
+  if (no_retry || downstream->no_more_retry()) {
+    goto fail;
+  }
+
+  // downstream connection is clean; we can retry with new
+  // downstream connection.
+
+  dconn = handler_->get_downstream_connection(downstream);
+  if (!dconn) {
+    goto fail;
+  }
+
+  rv = downstream->attach_downstream_connection(std::move(dconn));
+  if (rv != 0) {
+    goto fail;
+  }
+
+  rv = downstream->push_request_headers();
+  if (rv != 0) {
+    goto fail;
+  }
+
+  return 0;
+
+fail:
+  if (on_downstream_abort_request(downstream, 503) != 0) {
+    rst_stream(downstream, NGHTTP2_INTERNAL_ERROR);
+  }
+  downstream->pop_downstream_connection();
+
   handler_->signal_write();
 
   return 0;

src/shrpx_http2_upstream.h

@@ -78,7 +78,7 @@ public:
   virtual int on_downstream_body_complete(Downstream *downstream);
 
   virtual void on_handler_delete();
-  virtual int on_downstream_reset(bool no_retry);
+  virtual int on_downstream_reset(Downstream *downstream, bool no_retry);
   virtual int send_reply(Downstream *downstream, const uint8_t *body,
                          size_t bodylen);
   virtual int initiate_push(Downstream *downstream, const StringRef &uri);

src/shrpx_https_upstream.cc

@@ -1202,10 +1202,12 @@ void HttpsUpstream::on_handler_delete() {
   }
 }
 
-int HttpsUpstream::on_downstream_reset(bool no_retry) {
+int HttpsUpstream::on_downstream_reset(Downstream *downstream, bool no_retry) {
   int rv;
   std::unique_ptr<DownstreamConnection> dconn;
 
+  assert(downstream == downstream_.get());
+
   if (!downstream_->request_submission_ready()) {
     // Return error so that caller can delete handler
     return -1;

src/shrpx_https_upstream.h

@@ -73,7 +73,7 @@ public:
   virtual int on_downstream_body_complete(Downstream *downstream);
 
   virtual void on_handler_delete();
-  virtual int on_downstream_reset(bool no_retry);
+  virtual int on_downstream_reset(Downstream *downstream, bool no_retry);
   virtual int send_reply(Downstream *downstream, const uint8_t *body,
                          size_t bodylen);
   virtual int initiate_push(Downstream *downstream, const StringRef &uri);

src/shrpx_spdy_upstream.cc

@@ -1287,63 +1287,66 @@ void SpdyUpstream::on_handler_delete() {
   }
 }
 
-int SpdyUpstream::on_downstream_reset(bool no_retry) {
+int SpdyUpstream::on_downstream_reset(Downstream *downstream, bool no_retry) {
   int rv;
 
-  for (auto downstream = downstream_queue_.get_downstreams(); downstream;
-       downstream = downstream->dlnext) {
-    if (downstream->get_dispatch_state() != Downstream::DISPATCH_ACTIVE) {
-      // This is error condition when we failed push_request_headers()
-      // in initiate_downstream().  Otherwise, we have
-      // Downstream::DISPATCH_ACTIVE state, or we did not set
-      // DownstreamConnection.
-      downstream->pop_downstream_connection();
-      continue;
-    }
-
-    if (!downstream->request_submission_ready()) {
-      rst_stream(downstream, SPDYLAY_INTERNAL_ERROR);
-      downstream->pop_downstream_connection();
-      continue;
-    }
-
+  if (downstream->get_dispatch_state() != Downstream::DISPATCH_ACTIVE) {
+    // This is error condition when we failed push_request_headers()
+    // in initiate_downstream().  Otherwise, we have
+    // Downstream::DISPATCH_ACTIVE state, or we did not set
+    // DownstreamConnection.
     downstream->pop_downstream_connection();
 
-    downstream->add_retry();
+    handler_->signal_write();
 
-    std::unique_ptr<DownstreamConnection> dconn;
-
-    if (no_retry || downstream->no_more_retry()) {
-      goto fail;
-    }
-
-    // downstream connection is clean; we can retry with new
-    // downstream connection.
-
-    dconn = handler_->get_downstream_connection(downstream);
-    if (!dconn) {
-      goto fail;
-    }
-
-    rv = downstream->attach_downstream_connection(std::move(dconn));
-    if (rv != 0) {
-      goto fail;
-    }
-
-    rv = downstream->push_request_headers();
-    if (rv != 0) {
-      goto fail;
-    }
-
-    continue;
-
-  fail:
-    if (on_downstream_abort_request(downstream, 503) != 0) {
-      return -1;
-    }
-    downstream->pop_downstream_connection();
+    return 0;
   }
 
+  if (!downstream->request_submission_ready()) {
+    rst_stream(downstream, SPDYLAY_INTERNAL_ERROR);
+    downstream->pop_downstream_connection();
+
+    handler_->signal_write();
+
+    return 0;
+  }
+
+  downstream->pop_downstream_connection();
+
+  downstream->add_retry();
+
+  std::unique_ptr<DownstreamConnection> dconn;
+
+  if (no_retry || downstream->no_more_retry()) {
+    goto fail;
+  }
+
+  // downstream connection is clean; we can retry with new
+  // downstream connection.
+
+  dconn = handler_->get_downstream_connection(downstream);
+  if (!dconn) {
+    goto fail;
+  }
+
+  rv = downstream->attach_downstream_connection(std::move(dconn));
+  if (rv != 0) {
+    goto fail;
+  }
+
+  rv = downstream->push_request_headers();
+  if (rv != 0) {
+    goto fail;
+  }
+
+  return 0;
+
+fail:
+  if (on_downstream_abort_request(downstream, 503) != 0) {
+    rst_stream(downstream, SPDYLAY_INTERNAL_ERROR);
+  }
+  downstream->pop_downstream_connection();
+
   handler_->signal_write();
 
   return 0;

src/shrpx_spdy_upstream.h

@@ -72,7 +72,7 @@ public:
   virtual int on_downstream_body_complete(Downstream *downstream);
 
   virtual void on_handler_delete();
-  virtual int on_downstream_reset(bool no_retry);
+  virtual int on_downstream_reset(Downstream *downstream, bool no_retry);
 
   virtual int send_reply(Downstream *downstream, const uint8_t *body,
                          size_t bodylen);

src/shrpx_upstream.h

@@ -57,10 +57,11 @@ public:
   virtual int on_downstream_body_complete(Downstream *downstream) = 0;
 
   virtual void on_handler_delete() = 0;
-  // Called when downstream connection is reset.  Currently this is
-  // only used by Http2Session.  If |no_retry| is true, another
-  // connection attempt using new DownstreamConnection is not allowed.
-  virtual int on_downstream_reset(bool no_retry) = 0;
+  // Called when downstream connection for |downstream| is reset.
+  // Currently this is only used by Http2Session.  If |no_retry| is
+  // true, another connection attempt using new DownstreamConnection
+  // is not allowed.
+  virtual int on_downstream_reset(Downstream *downstream, bool no_retry) = 0;
 
   virtual void pause_read(IOCtrlReason reason) = 0;
   virtual int resume_read(IOCtrlReason reason, Downstream *downstream,