diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 6ac4fe395d78f..4bcceeaca978f 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -181,6 +181,12 @@ class Stream { * Cessation of data may not be immediate. For example, for HTTP/2 this may stop further flow * control window updates which will result in the peer eventually stopping sending data. * @param disable informs if reads should be disabled (true) or re-enabled (false). + * + * Note that this function reference counts calls. For example + * readDisable(true); // Disables data + * readDisable(true); // Notes the stream is blocked by two sources + * readDisable(false); // Notes the stream is blocked by one source + * readDisable(false); // Marks the stream as unblocked, so resumes reading. */ virtual void readDisable(bool disable) PURE; @@ -324,13 +330,19 @@ class DownstreamWatermarkCallbacks { virtual ~DownstreamWatermarkCallbacks() {} /** - * Called when the downstream connection or stream goes over its high watermark. + * Called when the downstream connection or stream goes over its high watermark. Note that this + * may be called separately for both the stream going over and the connection going over. It + * is the responsibility of the DownstreamWatermarkCallbacks implementation to handle unwinding + * multiple high and low watermark calls. */ virtual void onAboveWriteBufferHighWatermark() PURE; /** * Called when the downstream connection or stream goes from over its high watermark to under its - * low watermark. + * low watermark. As with onAboveWriteBufferHighWatermark above, this may be called independently + * when both the stream and the connection go under the low watermark limit, and the callee must + * ensure that the flow of data does not resume until all callers which were above their high + * watermarks have gone below. */ virtual void onBelowWriteBufferLowWatermark() PURE; }; diff --git a/include/envoy/network/connection.h b/include/envoy/network/connection.h index 3630fd34a400b..86d407bb3e485 100644 --- a/include/envoy/network/connection.h +++ b/include/envoy/network/connection.h @@ -144,6 +144,12 @@ class Connection : public Event::DeferredDeletable, public FilterManager { * enabled again if there is data still in the input buffer it will be redispatched through * the filter chain. * @param disable supplies TRUE is reads should be disabled, FALSE if they should be enabled. + * + * Note that this function reference counts calls. For example + * readDisable(true); // Disables data + * readDisable(true); // Notes the connection is blocked by two sources + * readDisable(false); // Notes the connection is blocked by one source + * readDisable(false); // Marks the connection as unblocked, so resumes reading. */ virtual void readDisable(bool disable) PURE; diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 47f40754ba81e..3b8fb95f6bc9c 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -1275,6 +1275,8 @@ void Filter::UpstreamRequest::DownstreamWatermarkManager::onAboveWriteBufferHigh ASSERT(parent_.request_encoder_); ASSERT(parent_.parent_.upstream_requests_.size() == 1); // The downstream connection is overrun. Pause reads from upstream. + // If there are multiple calls to readDisable either the codec (H2) or the underlying + // Network::Connection (H1) will handle reference counting. parent_.parent_.cluster_->stats().upstream_flow_control_paused_reading_total_.inc(); parent_.request_encoder_->getStream().readDisable(true); } @@ -1282,7 +1284,8 @@ void Filter::UpstreamRequest::DownstreamWatermarkManager::onAboveWriteBufferHigh void Filter::UpstreamRequest::DownstreamWatermarkManager::onBelowWriteBufferLowWatermark() { ASSERT(parent_.request_encoder_); ASSERT(parent_.parent_.upstream_requests_.size() == 1); - // The downstream connection has buffer available. Resume reads from upstream. + // One source of connection blockage has buffer available. Pass this on to the stream, which + // will resume reads if this was the last remaining high watermark. parent_.parent_.cluster_->stats().upstream_flow_control_resumed_reading_total_.inc(); parent_.request_encoder_->getStream().readDisable(false); } diff --git a/source/docs/flow_control.md b/source/docs/flow_control.md index 0d9e6172ede05..e3673ad2fe439 100644 --- a/source/docs/flow_control.md +++ b/source/docs/flow_control.md @@ -93,11 +93,13 @@ connection, the connection manager tracks how many outstanding high watermark events have occurred and passes any on to the router filter when it subscribes. It is worth noting that the router does not unwind `readDisable(true)` calls on -destruction. Each codec must ensure that any necessary readDisable calls are -unwound. In the case of HTTP/2 the `Envoy::Http::Http2::ConnectionImpl` will consume +destruction. In the case of HTTP/2 the `Envoy::Http::Http2::ConnectionImpl` will consume any outstanding flow control window on stream deletion to avoid leaking the connection-level -window. In the case of HTTP, the Envoy::Http::ConnectionManagerImpl unwinds any readDisable() -calls to ensure that pipelined requests will be read. +window. In the case of HTTP/1, the the Envoy::Http::ConnectionManagerImpl unwinds any readDisable() +calls downstream to ensure that pipelined requests will be read. For HTTP/1 +upstream connections, the `readDisable(true)` calls are unwound in +ClientConnectionImpl::onMessageComplete() to make sure that as connections are +returned to the connection pool they are ready to read. ## HTTP/2 codec recv buffer