From d1a1e882bf89f302652c37341365504ebc5559f7 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Tue, 17 Feb 2015 23:15:53 +0900 Subject: [PATCH] nghttpx: Fix request re-submission bug in HTTP/2 backend --- src/shrpx_downstream.cc | 12 +++++- src/shrpx_downstream.h | 8 ++++ src/shrpx_http2_downstream_connection.cc | 9 ++-- src/shrpx_http2_session.cc | 52 ++++++++---------------- src/shrpx_http2_session.h | 2 + src/shrpx_http2_upstream.cc | 4 +- src/shrpx_https_upstream.cc | 4 +- src/shrpx_spdy_upstream.cc | 4 +- 8 files changed, 47 insertions(+), 48 deletions(-) diff --git a/src/shrpx_downstream.cc b/src/shrpx_downstream.cc index 7622e1b2..eb574063 100644 --- a/src/shrpx_downstream.cc +++ b/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 diff --git a/src/shrpx_downstream.h b/src/shrpx_downstream.h index 810155ec..9a696380 100644 --- a/src/shrpx_downstream.h +++ b/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 diff --git a/src/shrpx_http2_downstream_connection.cc b/src/shrpx_http2_downstream_connection.cc index 74bdcc8e..f580f77f 100644 --- a/src/shrpx_http2_downstream_connection.cc +++ b/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 && diff --git a/src/shrpx_http2_session.cc b/src/shrpx_http2_session.cc index eb870bb5..232861b0 100644 --- a/src/shrpx_http2_session.cc +++ b/src/shrpx_http2_session.cc @@ -113,11 +113,12 @@ void writecb(struct ev_loop *loop, ev_io *w, int revents) { auto conn = static_cast(w->data); auto http2session = static_cast(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); } } diff --git a/src/shrpx_http2_session.h b/src/shrpx_http2_session.h index 1fc48524..f1a288a0 100644 --- a/src/shrpx_http2_session.h +++ b/src/shrpx_http2_session.h @@ -141,6 +141,8 @@ public: bool should_hard_fail() const; + void submit_pending_requests(); + enum { // Disconnected DISCONNECTED, diff --git a/src/shrpx_http2_upstream.cc b/src/shrpx_http2_upstream.cc index 3564c6d3..14e313e3 100644 --- a/src/shrpx_http2_upstream.cc +++ b/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; diff --git a/src/shrpx_https_upstream.cc b/src/shrpx_https_upstream.cc index 1c799c7e..4406f437 100644 --- a/src/shrpx_https_upstream.cc +++ b/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; } diff --git a/src/shrpx_spdy_upstream.cc b/src/shrpx_spdy_upstream.cc index 4e76f683..a39d8f8b 100644 --- a/src/shrpx_spdy_upstream.cc +++ b/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;