From b305495a75ab112b7a3ca3f0662c59f00c589dbc Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 18 Sep 2014 23:03:36 +0900 Subject: [PATCH] nghttpx: Reset both timeouts when either read or write succeeds Previously read and write timeouts work independently. When we are writing response to the client, read timeout still ticks (e.g., HTTP/2 or tunneled HTTPS connection). So read timeout may occur during long download. This commit fixes this issue. This commit only fixes the upstream part. We need similar fix for the downstream. --- src/shrpx_client_handler.cc | 10 +++++++++- src/shrpx_http2_upstream.cc | 9 +++++++-- src/shrpx_http2_upstream.h | 2 ++ src/shrpx_https_upstream.cc | 6 ++++++ src/shrpx_https_upstream.h | 2 ++ src/shrpx_spdy_upstream.cc | 9 +++++++-- src/shrpx_spdy_upstream.h | 2 ++ src/shrpx_upstream.h | 2 ++ 8 files changed, 37 insertions(+), 5 deletions(-) diff --git a/src/shrpx_client_handler.cc b/src/shrpx_client_handler.cc index 3ec56d5a..de7e8d48 100644 --- a/src/shrpx_client_handler.cc +++ b/src/shrpx_client_handler.cc @@ -49,6 +49,10 @@ namespace { void upstream_readcb(bufferevent *bev, void *arg) { auto handler = static_cast(arg); + auto upstream = handler->get_upstream(); + if(upstream) { + upstream->reset_timeouts(); + } int rv = handler->on_read(); if(rv != 0) { delete handler; @@ -60,6 +64,10 @@ namespace { void upstream_writecb(bufferevent *bev, void *arg) { auto handler = static_cast(arg); + auto upstream = handler->get_upstream(); + if(upstream) { + upstream->reset_timeouts(); + } // We actually depend on write low-water mark == 0. if(handler->get_outbuf_length() > 0) { @@ -71,7 +79,7 @@ void upstream_writecb(bufferevent *bev, void *arg) delete handler; return; } - auto upstream = handler->get_upstream(); + if(!upstream) { return; } diff --git a/src/shrpx_http2_upstream.cc b/src/shrpx_http2_upstream.cc index 56d7abf3..a6caf982 100644 --- a/src/shrpx_http2_upstream.cc +++ b/src/shrpx_http2_upstream.cc @@ -627,8 +627,7 @@ Http2Upstream::Http2Upstream(ClientHandler *handler) session_(nullptr), settings_timerev_(nullptr) { - handler->set_upstream_timeouts(&get_config()->http2_upstream_read_timeout, - &get_config()->upstream_write_timeout); + reset_timeouts(); int rv; @@ -1436,4 +1435,10 @@ int Http2Upstream::on_timeout(Downstream *downstream) return 0; } +void Http2Upstream::reset_timeouts() +{ + handler_->set_upstream_timeouts(&get_config()->http2_upstream_read_timeout, + &get_config()->upstream_write_timeout); +} + } // namespace shrpx diff --git a/src/shrpx_http2_upstream.h b/src/shrpx_http2_upstream.h index 2c7739d6..4167ad99 100644 --- a/src/shrpx_http2_upstream.h +++ b/src/shrpx_http2_upstream.h @@ -73,6 +73,8 @@ public: const uint8_t *data, size_t len, bool flush); virtual int on_downstream_body_complete(Downstream *downstream); + virtual void reset_timeouts(); + bool get_flow_control() const; // Perform HTTP/2 upgrade from |upstream|. On success, this object // takes ownership of the |upstream|. This function returns 0 if it diff --git a/src/shrpx_https_upstream.cc b/src/shrpx_https_upstream.cc index 60de84cc..3c55ceab 100644 --- a/src/shrpx_https_upstream.cc +++ b/src/shrpx_https_upstream.cc @@ -973,4 +973,10 @@ void HttpsUpstream::log_response_headers(const std::string& hdrs) const ULOG(INFO, this) << "HTTP response headers\n" << hdrp; } +void HttpsUpstream::reset_timeouts() +{ + handler_->set_upstream_timeouts(&get_config()->upstream_read_timeout, + &get_config()->upstream_write_timeout); +} + } // namespace shrpx diff --git a/src/shrpx_https_upstream.h b/src/shrpx_https_upstream.h index c7f46ae5..226c0305 100644 --- a/src/shrpx_https_upstream.h +++ b/src/shrpx_https_upstream.h @@ -67,6 +67,8 @@ public: const uint8_t *data, size_t len, bool flush); virtual int on_downstream_body_complete(Downstream *downstream); + virtual void reset_timeouts(); + void reset_current_header_length(); void log_response_headers(const std::string& hdrs) const; private: diff --git a/src/shrpx_spdy_upstream.cc b/src/shrpx_spdy_upstream.cc index f9926cee..bbfbcfa8 100644 --- a/src/shrpx_spdy_upstream.cc +++ b/src/shrpx_spdy_upstream.cc @@ -437,8 +437,7 @@ SpdyUpstream::SpdyUpstream(uint16_t version, ClientHandler *handler) session_(nullptr) { //handler->set_bev_cb(spdy_readcb, 0, spdy_eventcb); - handler->set_upstream_timeouts(&get_config()->http2_upstream_read_timeout, - &get_config()->upstream_write_timeout); + reset_timeouts(); spdylay_session_callbacks callbacks; memset(&callbacks, 0, sizeof(callbacks)); @@ -1129,4 +1128,10 @@ int SpdyUpstream::on_timeout(Downstream *downstream) return 0; } +void SpdyUpstream::reset_timeouts() +{ + handler_->set_upstream_timeouts(&get_config()->http2_upstream_read_timeout, + &get_config()->upstream_write_timeout); +} + } // namespace shrpx diff --git a/src/shrpx_spdy_upstream.h b/src/shrpx_spdy_upstream.h index 44e6d67e..113f2e4a 100644 --- a/src/shrpx_spdy_upstream.h +++ b/src/shrpx_spdy_upstream.h @@ -72,6 +72,8 @@ public: const uint8_t *data, size_t len, bool flush); virtual int on_downstream_body_complete(Downstream *downstream); + virtual void reset_timeouts(); + bool get_flow_control() const; int consume(int32_t stream_id, size_t len); diff --git a/src/shrpx_upstream.h b/src/shrpx_upstream.h index 758e5563..fb623a25 100644 --- a/src/shrpx_upstream.h +++ b/src/shrpx_upstream.h @@ -59,6 +59,8 @@ public: virtual void pause_read(IOCtrlReason reason) = 0; virtual int resume_read(IOCtrlReason reason, Downstream *downstream, size_t consumed) = 0; + + virtual void reset_timeouts() = 0; }; } // namespace shrpx