From a0ce5ea9aba0ae8801f3911b4765a84ffc9b42d5 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sat, 24 Dec 2016 22:50:02 +0900 Subject: [PATCH 1/2] nghttpx: Keep reading after backend write failed Because of bidirectional nature of TCP, we may fail write(2), but have still pending read in TCP buffer, which may contain response body. To forward them, we should keep reading until get EOF from backend. To avoid stalling HTTP/1 upload when request buffer is full, and we have received complete response from backend, drop connection in that case. --- src/shrpx_http2_session.cc | 14 ++++++++++++-- src/shrpx_http_downstream_connection.cc | 14 ++++++++++++-- src/shrpx_https_upstream.cc | 4 +++- 3 files changed, 27 insertions(+), 5 deletions(-) diff --git a/src/shrpx_http2_session.cc b/src/shrpx_http2_session.cc index e2b4608c..24d9cbd5 100644 --- a/src/shrpx_http2_session.cc +++ b/src/shrpx_http2_session.cc @@ -2022,7 +2022,12 @@ int Http2Session::write_clear() { } if (nwrite < 0) { - return nwrite; + // We may have pending data in receive buffer which may + // contain part of response body. So keep reading. Invoke + // read event to get read(2) error just in case. + ev_feed_event(conn_.loop, &conn_.rev, EV_READ); + wb_.drain(wb_.rleft()); + break; } wb_.drain(nwrite); @@ -2132,7 +2137,12 @@ int Http2Session::write_tls() { } if (nwrite < 0) { - return nwrite; + // We may have pending data in receive buffer which may + // contain part of response body. So keep reading. Invoke + // read event to get read(2) error just in case. + ev_feed_event(conn_.loop, &conn_.rev, EV_READ); + wb_.drain(wb_.rleft()); + break; } wb_.drain(nwrite); diff --git a/src/shrpx_http_downstream_connection.cc b/src/shrpx_http_downstream_connection.cc index 70333a09..f6cf7998 100644 --- a/src/shrpx_http_downstream_connection.cc +++ b/src/shrpx_http_downstream_connection.cc @@ -1116,7 +1116,12 @@ int HttpDownstreamConnection::write_clear() { } if (nwrite < 0) { - return nwrite; + // We may have pending data in receive buffer which may contain + // part of response body. So keep reading. Invoke read event + // to get read(2) error just in case. + ev_feed_event(conn_.loop, &conn_.rev, EV_READ); + input->drain(input->rleft()); + break; } input->drain(nwrite); @@ -1236,7 +1241,12 @@ int HttpDownstreamConnection::write_tls() { } if (nwrite < 0) { - return nwrite; + // We may have pending data in receive buffer which may contain + // part of response body. So keep reading. Invoke read event + // to get read(2) error just in case. + ev_feed_event(conn_.loop, &conn_.rev, EV_READ); + input->drain(input->rleft()); + break; } input->drain(nwrite); diff --git a/src/shrpx_https_upstream.cc b/src/shrpx_https_upstream.cc index a12d1a40..4ea06aba 100644 --- a/src/shrpx_https_upstream.cc +++ b/src/shrpx_https_upstream.cc @@ -1184,7 +1184,9 @@ int HttpsUpstream::on_downstream_body_complete(Downstream *downstream) { resp.connection_close = true; } - if (req.connection_close || resp.connection_close) { + if (req.connection_close || resp.connection_close || + // To avoid to stall upload body + downstream->get_request_state() != Downstream::MSG_COMPLETE) { auto handler = get_client_handler(); handler->set_should_close_after_write(true); } From cd83d70e7b23da166488953a26e7255003ab43bb Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sat, 24 Dec 2016 22:54:22 +0900 Subject: [PATCH 2/2] nghttpx: Don't reset stream if we have already received response --- src/shrpx_http2_upstream.cc | 5 +++++ src/shrpx_https_upstream.cc | 14 ++++++++++++-- src/shrpx_spdy_upstream.cc | 5 +++++ 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/shrpx_http2_upstream.cc b/src/shrpx_http2_upstream.cc index b1882abf..ebb762d5 100644 --- a/src/shrpx_http2_upstream.cc +++ b/src/shrpx_http2_upstream.cc @@ -1854,6 +1854,11 @@ int Http2Upstream::on_downstream_reset(Downstream *downstream, bool no_retry) { } if (!downstream->request_submission_ready()) { + if (downstream->get_response_state() == Downstream::MSG_COMPLETE) { + // We have got all response body already. Send it off. + downstream->pop_downstream_connection(); + return 0; + } // pushed stream is handled here rst_stream(downstream, NGHTTP2_INTERNAL_ERROR); downstream->pop_downstream_connection(); diff --git a/src/shrpx_https_upstream.cc b/src/shrpx_https_upstream.cc index 4ea06aba..bc939f2f 100644 --- a/src/shrpx_https_upstream.cc +++ b/src/shrpx_https_upstream.cc @@ -1223,13 +1223,23 @@ int HttpsUpstream::on_downstream_reset(Downstream *downstream, bool no_retry) { assert(downstream == downstream_.get()); + downstream_->pop_downstream_connection(); + if (!downstream_->request_submission_ready()) { + switch (downstream_->get_response_state()) { + case Downstream::MSG_COMPLETE: + // We have got all response body already. Send it off. + return 0; + case Downstream::INITIAL: + if (on_downstream_abort_request(downstream_.get(), 503) != 0) { + return -1; + } + return 0; + } // Return error so that caller can delete handler return -1; } - downstream_->pop_downstream_connection(); - downstream_->add_retry(); if (no_retry || downstream_->no_more_retry()) { diff --git a/src/shrpx_spdy_upstream.cc b/src/shrpx_spdy_upstream.cc index eff888be..3e50a62d 100644 --- a/src/shrpx_spdy_upstream.cc +++ b/src/shrpx_spdy_upstream.cc @@ -1314,6 +1314,11 @@ int SpdyUpstream::on_downstream_reset(Downstream *downstream, bool no_retry) { } if (!downstream->request_submission_ready()) { + if (downstream->get_response_state() == Downstream::MSG_COMPLETE) { + // We have got all response body already. Send it off. + downstream->pop_downstream_connection(); + return 0; + } rst_stream(downstream, SPDYLAY_INTERNAL_ERROR); downstream->pop_downstream_connection();