From 8bac5899cc169129c8b5a04647f116b80ecfdb50 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 14 Sep 2016 22:16:07 +0900 Subject: [PATCH] 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 | 29 +++++----- src/shrpx_http2_upstream.cc | 106 ++++++++++++++++++------------------ src/shrpx_http2_upstream.h | 2 +- src/shrpx_https_upstream.cc | 4 +- src/shrpx_https_upstream.h | 2 +- src/shrpx_spdy_upstream.cc | 103 ++++++++++++++++++----------------- src/shrpx_spdy_upstream.h | 2 +- src/shrpx_upstream.h | 9 +-- 8 files changed, 134 insertions(+), 123 deletions(-) diff --git a/src/shrpx_http2_session.cc b/src/shrpx_http2_session.cc index 99179aad..9c258a32 100644 --- a/src/shrpx_http2_session.cc +++ b/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 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_); diff --git a/src/shrpx_http2_upstream.cc b/src/shrpx_http2_upstream.cc index 565deced..112f41ce 100644 --- a/src/shrpx_http2_upstream.cc +++ b/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 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 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; diff --git a/src/shrpx_http2_upstream.h b/src/shrpx_http2_upstream.h index f9c8fb82..84cdea0c 100644 --- a/src/shrpx_http2_upstream.h +++ b/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); diff --git a/src/shrpx_https_upstream.cc b/src/shrpx_https_upstream.cc index 6332b29f..0be62b62 100644 --- a/src/shrpx_https_upstream.cc +++ b/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 dconn; + assert(downstream == downstream_.get()); + if (!downstream_->request_submission_ready()) { // Return error so that caller can delete handler return -1; diff --git a/src/shrpx_https_upstream.h b/src/shrpx_https_upstream.h index 0f9b8dd0..4544651c 100644 --- a/src/shrpx_https_upstream.h +++ b/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); diff --git a/src/shrpx_spdy_upstream.cc b/src/shrpx_spdy_upstream.cc index 7b17c4c2..8077d426 100644 --- a/src/shrpx_spdy_upstream.cc +++ b/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 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 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; diff --git a/src/shrpx_spdy_upstream.h b/src/shrpx_spdy_upstream.h index 2d6bd244..af5f589a 100644 --- a/src/shrpx_spdy_upstream.h +++ b/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); diff --git a/src/shrpx_upstream.h b/src/shrpx_upstream.h index f85bb776..bc013aa1 100644 --- a/src/shrpx_upstream.h +++ b/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,