From fe6bc035a93ffa685eafe1f39b9a6e2749713713 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E6=96=B9=E4=BF=8A=E6=AD=A6?= <fangjunwu@xunlei.com>
Date: Thu, 18 May 2017 11:18:57 +0800
Subject: [PATCH] Remove MAX_STREAMS check on client side connection pool.
 (#963)

The reason for this is that we already have protection via circuit breakers.
There is no point in having additional protection (other than potentially
for push, which we don't support currently, and can deal with later). If we
overflow the other side, the code will correctly handle the REFUSED_STREAM
reset that happens.
---
 source/common/http/http2/conn_pool.cc    | 27 ++++++++----------------
 source/common/http/http2/conn_pool.h     |  2 --
 test/common/http/http2/conn_pool_test.cc | 22 -------------------
 3 files changed, 9 insertions(+), 42 deletions(-)

diff --git a/source/common/http/http2/conn_pool.cc b/source/common/http/http2/conn_pool.cc
index 21a276c807e46..42b26904e5e30 100644
--- a/source/common/http/http2/conn_pool.cc
+++ b/source/common/http/http2/conn_pool.cc
@@ -82,22 +82,15 @@ ConnectionPool::Cancellable* ConnPoolImpl::newStream(Http::StreamDecoder& respon
     primary_client_.reset(new ActiveClient(*this));
   }
 
-  if (primary_client_->client_->numActiveRequests() >= maxConcurrentStreams() ||
-      !host_->cluster().resourceManager(priority_).requests().canCreate()) {
-    log_debug("max requests overflow");
-    callbacks.onPoolFailure(ConnectionPool::PoolFailureReason::Overflow, nullptr);
-    host_->cluster().stats().upstream_rq_pending_overflow_.inc();
-  } else {
-    conn_log_debug("creating stream", *primary_client_->client_);
-    primary_client_->total_streams_++;
-    host_->stats().rq_total_.inc();
-    host_->stats().rq_active_.inc();
-    host_->cluster().stats().upstream_rq_total_.inc();
-    host_->cluster().stats().upstream_rq_active_.inc();
-    host_->cluster().resourceManager(priority_).requests().inc();
-    callbacks.onPoolReady(primary_client_->client_->newStream(response_decoder),
-                          primary_client_->real_host_description_);
-  }
+  conn_log_debug("creating stream", *primary_client_->client_);
+  primary_client_->total_streams_++;
+  host_->stats().rq_total_.inc();
+  host_->stats().rq_active_.inc();
+  host_->cluster().stats().upstream_rq_total_.inc();
+  host_->cluster().stats().upstream_rq_active_.inc();
+  host_->cluster().resourceManager(priority_).requests().inc();
+  callbacks.onPoolReady(primary_client_->client_->newStream(response_decoder),
+                        primary_client_->real_host_description_);
 
   return nullptr;
 }
@@ -247,8 +240,6 @@ CodecClientPtr ProdConnPoolImpl::createCodecClient(Upstream::Host::CreateConnect
   return codec;
 }
 
-uint64_t ProdConnPoolImpl::maxConcurrentStreams() { return ConnectionImpl::MAX_CONCURRENT_STREAMS; }
-
 uint32_t ProdConnPoolImpl::maxTotalStreams() { return MAX_STREAMS; }
 
 } // Http2
diff --git a/source/common/http/http2/conn_pool.h b/source/common/http/http2/conn_pool.h
index f188744260dc2..bb66b5430a4fb 100644
--- a/source/common/http/http2/conn_pool.h
+++ b/source/common/http/http2/conn_pool.h
@@ -66,7 +66,6 @@ class ConnPoolImpl : Logger::Loggable<Logger::Id::pool>, public ConnectionPool::
 
   void checkForDrained();
   virtual CodecClientPtr createCodecClient(Upstream::Host::CreateConnectionData& data) PURE;
-  virtual uint64_t maxConcurrentStreams() PURE;
   virtual uint32_t maxTotalStreams() PURE;
   void movePrimaryClientToDraining();
   void onConnectionEvent(ActiveClient& client, uint32_t events);
@@ -93,7 +92,6 @@ class ProdConnPoolImpl : public ConnPoolImpl {
 
 private:
   CodecClientPtr createCodecClient(Upstream::Host::CreateConnectionData& data) override;
-  uint64_t maxConcurrentStreams() override;
   uint32_t maxTotalStreams() override;
 
   // All streams are 2^31. Client streams are half that, minus stream 0. Just to be on the safe
diff --git a/test/common/http/http2/conn_pool_test.cc b/test/common/http/http2/conn_pool_test.cc
index 1ca1eb0973e8f..c94ea6e6c38cd 100644
--- a/test/common/http/http2/conn_pool_test.cc
+++ b/test/common/http/http2/conn_pool_test.cc
@@ -43,10 +43,8 @@ class TestConnPoolImpl : public ConnPoolImpl {
 
   MOCK_METHOD1(createCodecClient_, CodecClient*(Upstream::Host::CreateConnectionData& data));
 
-  uint64_t maxConcurrentStreams() override { return max_concurrent_streams_; }
   uint32_t maxTotalStreams() override { return max_streams_; }
 
-  uint64_t max_concurrent_streams_{std::numeric_limits<uint64_t>::max()};
   uint32_t max_streams_{std::numeric_limits<uint32_t>::max()};
 };
 
@@ -356,26 +354,6 @@ TEST_F(Http2ConnPoolImplTest, ConnectTimeout) {
   EXPECT_EQ(1U, cluster_->stats_.upstream_rq_pending_failure_eject_.value());
 }
 
-TEST_F(Http2ConnPoolImplTest, MaxRequests) {
-  InSequence s;
-  pool_.max_concurrent_streams_ = 1;
-
-  expectClientCreate();
-  ActiveTestRequest r1(*this, 0);
-  EXPECT_CALL(r1.inner_encoder_, encodeHeaders(_, true));
-  r1.callbacks_.outer_encoder_->encodeHeaders(HeaderMapImpl{}, true);
-  expectClientConnect(0);
-
-  ConnPoolCallbacks callbacks;
-  Http::MockStreamDecoder decoder;
-  EXPECT_CALL(callbacks.pool_failure_, ready());
-  EXPECT_EQ(nullptr, pool_.newStream(decoder, callbacks));
-
-  test_clients_[0].connection_->raiseEvents(Network::ConnectionEvent::RemoteClose);
-  EXPECT_CALL(*this, onClientDestroy());
-  dispatcher_.clearDeferredDeleteList();
-}
-
 TEST_F(Http2ConnPoolImplTest, MaxGlobalRequests) {
   InSequence s;
   cluster_->resource_manager_.reset(