docs: improving watermark docs/comments (#6683)

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

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;
 };

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;
 

source/common/router/router.cc

@@ -1275,14 +1275,17 @@ 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);
 }
 
 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);
 }

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