http2: remove unnecessary negative_capacity_ (#19057)

Commit Message: remove negative_capacity_ to make the code easier to understand
Additional Description: fixes part I of #18880
Risk Level: high
Testing: unit
Docs Changes: n/a

Signed-off-by: YaoZengzeng <[email protected]>

source/common/conn_pool/conn_pool_base.cc

@@ -208,22 +208,25 @@ void ConnPoolImplBase::onStreamClosed(Envoy::ConnectionPool::ActiveClient& clien
                                       bool delay_attaching_stream) {
   ENVOY_CONN_LOG(debug, "destroying stream: {} remaining", client, client.numActiveStreams());
   ASSERT(num_active_streams_ > 0);
-  // Reflect there's one less stream in flight.
-  bool had_negative_capacity = client.hadNegativeDeltaOnStreamClosed();
   state_.decrActiveStreams(1);
   num_active_streams_--;
   host_->stats().rq_active_.dec();
   host_->cluster().stats().upstream_rq_active_.dec();
   host_->cluster().resourceManager(priority_).requests().dec();
-  // If the effective client capacity was limited by concurrency, increase connecting capacity.
-  // If the effective client capacity was limited by max total streams, this will not result in an
-  // increment as no capacity is freed up.
   // We don't update the capacity for HTTP/3 as the stream count should only
   // increase when a MAX_STREAMS frame is received.
-  if (trackStreamCapacity() && (client.remaining_streams_ > client.concurrent_stream_limit_ -
-                                                                client.numActiveStreams() - 1 ||
-                                had_negative_capacity)) {
-    state_.incrConnectingAndConnectedStreamCapacity(1);
+  if (trackStreamCapacity()) {
+    // If the effective client capacity was limited by concurrency, increase connecting capacity.
+    bool limited_by_concurrency =
+        client.remaining_streams_ > client.concurrent_stream_limit_ - client.numActiveStreams() - 1;
+    // The capacity calculated by concurrency could be negative if a SETTINGS frame lowered the
+    // number of allowed streams. In this case, effective client capacity was still limited by
+    // concurrency, compare client.concurrent_stream_limit_ and client.numActiveStreams() directly
+    // to avoid overflow.
+    bool negative_capacity = client.concurrent_stream_limit_ < client.numActiveStreams() + 1;
+    if (negative_capacity || limited_by_concurrency) {
+      state_.incrConnectingAndConnectedStreamCapacity(1);
+    }
   }
   if (client.state() == ActiveClient::State::DRAINING && client.numActiveStreams() == 0) {
     // Close out the draining client if we no longer have active streams.

source/common/http/conn_pool_base.cc

@@ -127,7 +127,6 @@ void MultiplexedActiveClientBase::onSettings(ReceivedSettings& settings) {
     }
     parent_.decrClusterStreamCapacity(delta);
     ENVOY_CONN_LOG(trace, "Decreasing stream capacity by {}", *codec_client_, delta);
-    negative_capacity_ += delta;
   }
   // As we don't increase stream limits when maxConcurrentStreams goes up, treat
   // a stream limit of 0 as a GOAWAY.

source/common/http/conn_pool_base.h

@@ -201,19 +201,6 @@ class MultiplexedActiveClientBase : public CodecClientCallbacks,
   void onGoAway(Http::GoAwayErrorCode error_code) override;
   void onSettings(ReceivedSettings& settings) override;
 
-  // As this is called once when the stream is closed, it's a good place to
-  // update the counter as one stream has been "returned" and the negative
-  // capacity should be reduced.
-  bool hadNegativeDeltaOnStreamClosed() override {
-    int ret = negative_capacity_ != 0;
-    if (negative_capacity_ > 0) {
-      negative_capacity_--;
-    }
-    return ret;
-  }
-
-  uint64_t negative_capacity_{};
-
 protected:
   MultiplexedActiveClientBase(Envoy::Http::HttpConnPoolImplBase& parent,
                               Upstream::Host::CreateConnectionData& data,

test/common/http/http2/conn_pool_test.cc

@@ -1669,6 +1669,47 @@ TEST_F(Http2ConnPoolImplTest, MaybePreconnect) {
   closeAllClients();
 }
 
+TEST_F(Http2ConnPoolImplTest, TestUnusedCapacity) {
+  cluster_->http2_options_.mutable_max_concurrent_streams()->set_value(8);
+  cluster_->max_requests_per_connection_ = 6;
+
+  expectClientsCreate(1);
+  ActiveTestRequest r1(*this, 0, false);
+  // Initially, capacity is based on remaining streams and capped at 6.
+  CHECK_STATE(0 /*active*/, 1 /*pending*/, 6 /*capacity*/);
+  expectClientConnect(0, r1);
+  // Now the stream is active, remaining concurrency capacity is 5.
+  CHECK_STATE(1 /*active*/, 0 /*pending*/, 5 /*capacity*/);
+
+  // With two more streams, remaining unused capacity is 3.
+  ActiveTestRequest r2(*this, 0, true);
+  ActiveTestRequest r3(*this, 0, true);
+  CHECK_STATE(3 /*active*/, 0 /*pending*/, 3 /*capacity*/);
+
+  // Settings frame results in 1 unused capacity.
+  NiceMock<MockReceivedSettings> settings;
+  settings.max_concurrent_streams_ = 4;
+  test_clients_[0].codec_client_->onSettings(settings);
+  CHECK_STATE(3 /*active*/, 0 /*pending*/, 1 /*capacity*/);
+
+  // Closing a stream, unused capacity returns to 2.
+  completeRequest(r1);
+  CHECK_STATE(2 /*active*/, 0 /*pending*/, 2 /*capacity*/);
+
+  // Closing another, unused capacity returns to 3 (3 remaining stream).
+  completeRequest(r2);
+  CHECK_STATE(1 /*active*/, 0 /*pending*/, 3 /*capacity*/);
+
+  // Closing the last stream, unused capacity remains at 3, as there is only 3 remaining streams.
+  completeRequest(r3);
+  CHECK_STATE(0 /*active*/, 0 /*pending*/, 3 /*capacity*/);
+
+  // Clean up with an outstanding stream.
+  pool_->drainConnections(Envoy::ConnectionPool::DrainBehavior::DrainExistingConnections);
+  closeAllClients();
+  CHECK_STATE(0 /*active*/, 0 /*pending*/, 0 /*capacity*/);
+}
+
 TEST_F(Http2ConnPoolImplTest, TestStateWithMultiplexing) {
   cluster_->http2_options_.mutable_max_concurrent_streams()->set_value(2);
   cluster_->max_requests_per_connection_ = 4;
@@ -1678,22 +1719,22 @@ TEST_F(Http2ConnPoolImplTest, TestStateWithMultiplexing) {
   // Initially, capacity is based on concurrency and capped at 2.
   CHECK_STATE(0 /*active*/, 1 /*pending*/, 2 /*capacity*/);
   expectClientConnect(0, r1);
-  // Now the stream is active, remaining concurrency capacity is 1
+  // Now the stream is active, remaining concurrency capacity is 1.
   CHECK_STATE(1 /*active*/, 0 /*pending*/, 1 /*capacity*/);
 
   // With one more stream, remaining concurrency capacity is 0.
   ActiveTestRequest r2(*this, 0, true);
   CHECK_STATE(2 /*active*/, 0 /*pending*/, 0 /*capacity*/);
 
-  // If one stream closes, concurrency capacity goes to 1 (2 remaining streams)
+  // If one stream closes, concurrency capacity goes to 1 (2 remaining streams).
   completeRequest(r1);
   CHECK_STATE(1 /*active*/, 0 /*pending*/, 1 /*capacity*/);
 
-  // Assigning a new stream, concurrency capacity returns to 0 (1 remaining stream);
+  // Assigning a new stream, concurrency capacity returns to 0 (1 remaining stream).
   ActiveTestRequest r3(*this, 0, true);
   CHECK_STATE(2 /*active*/, 0 /*pending*/, 0 /*capacity*/);
 
-  // Closing a stream, capacity returns to 1 (both concurrency and remaining streams)
+  // Closing a stream, capacity returns to 1 (both concurrency and remaining streams).
   completeRequest(r2);
   CHECK_STATE(1 /*active*/, 0 /*pending*/, 1 /*capacity*/);
 

tools/spelling/spelling_dictionary.txt

@@ -868,6 +868,7 @@ nan
 nanos
 natively
 ndk
+negative
 netblock
 netblocks
 netfilter