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,