From b48aee6b30c918a7320942c65afbbaec6b80b69e Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Mon, 30 Nov 2020 16:08:17 -0500 Subject: [PATCH 01/17] fix Signed-off-by: Asra Ali --- source/common/http/conn_pool_base.cc | 3 +- source/common/http/conn_pool_base.h | 4 ++ source/common/http/http1/conn_pool.cc | 5 +- test/common/http/http1/conn_pool_test.cc | 78 ++++++++++++++++++++---- 4 files changed, 77 insertions(+), 13 deletions(-) diff --git a/source/common/http/conn_pool_base.cc b/source/common/http/conn_pool_base.cc index 19c057a53edb..3ffa8fb9bb45 100644 --- a/source/common/http/conn_pool_base.cc +++ b/source/common/http/conn_pool_base.cc @@ -55,7 +55,8 @@ HttpConnPoolImplBase::HttpConnPoolImplBase( : Envoy::ConnectionPool::ConnPoolImplBase( host, priority, dispatcher, options, wrapTransportSocketOptions(transport_socket_options, protocols), state), - random_generator_(random_generator) { + random_generator_(random_generator), + upstream_ready_cb_(dispatcher_.createSchedulableCallback([this]() { onUpstreamReady(); })) { ASSERT(!protocols.empty()); } diff --git a/source/common/http/conn_pool_base.h b/source/common/http/conn_pool_base.h index 152dc6df86d5..7968bee68ebe 100644 --- a/source/common/http/conn_pool_base.h +++ b/source/common/http/conn_pool_base.h @@ -80,10 +80,14 @@ class HttpConnPoolImplBase : public Envoy::ConnectionPool::ConnPoolImplBase, virtual CodecClientPtr createCodecClient(Upstream::Host::CreateConnectionData& data) PURE; Random::RandomGenerator& randomGenerator() { return random_generator_; } + Event::SchedulableCallback* upstreamReadyCallback() { return upstream_ready_cb_.get(); } protected: friend class ActiveClient; Random::RandomGenerator& random_generator_; + + // onUpstreamReady callback only used by HTTP/1.1 implementation. + Event::SchedulableCallbackPtr upstream_ready_cb_; }; // An implementation of Envoy::ConnectionPool::ActiveClient for HTTP/1.1 and HTTP/2 diff --git a/source/common/http/http1/conn_pool.cc b/source/common/http/http1/conn_pool.cc index cc69ab976c0e..a6afd859be09 100644 --- a/source/common/http/http1/conn_pool.cc +++ b/source/common/http/http1/conn_pool.cc @@ -76,7 +76,10 @@ void ActiveClient::StreamWrapper::onDecodeComplete() { parent_.codec_client_->close(); } else { auto* pool = &parent_.parent(); - pool->dispatcher().post([pool]() -> void { pool->onUpstreamReady(); }); + if (pool->hasPendingStreams()) { + // SchedulableCallback guarantees callback will only be scheduled if not already enabled. + pool->upstreamReadyCallback()->scheduleCallbackCurrentIteration(); + } parent_.stream_wrapper_.reset(); pool->checkForDrained(); diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index b86e9c426a31..1e20cd7b7999 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -52,7 +52,8 @@ class ConnPoolImplForTest : public Event::TestUsingSimulatedTime, public FixedHt public: ConnPoolImplForTest(Event::MockDispatcher& dispatcher, Upstream::ClusterInfoConstSharedPtr cluster, - Random::RandomGenerator& random_generator) + Random::RandomGenerator& random_generator, + NiceMock* upstream_ready_cb) : FixedHttpConnPoolImpl( Upstream::makeTestHost(cluster, "tcp://127.0.0.1:9000", dispatcher.timeSource()), Upstream::ResourcePriority::Default, dispatcher, nullptr, nullptr, random_generator, @@ -62,7 +63,8 @@ class ConnPoolImplForTest : public Event::TestUsingSimulatedTime, public FixedHt return nullptr; // Not used: createCodecClient overloaded. }, std::vector{Protocol::Http11}), - api_(Api::createApiForTest()), mock_dispatcher_(dispatcher) {} + api_(Api::createApiForTest()), mock_dispatcher_(dispatcher), + mock_upstream_ready_cb_(upstream_ready_cb) {} ~ConnPoolImplForTest() override { EXPECT_EQ(0U, ready_clients_.size()); @@ -118,15 +120,17 @@ class ConnPoolImplForTest : public Event::TestUsingSimulatedTime, public FixedHt } void expectEnableUpstreamReady() { - EXPECT_CALL(mock_dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb_)); + EXPECT_CALL(*mock_upstream_ready_cb_, scheduleCallbackCurrentIteration()) + .Times(1) + .RetiresOnSaturation(); } - void expectAndRunUpstreamReady() { post_cb_(); } + void expectAndRunUpstreamReady() { mock_upstream_ready_cb_->invokeCallback(); } Upstream::ClusterConnectivityState state_; Api::ApiPtr api_; Event::MockDispatcher& mock_dispatcher_; - Event::PostCb post_cb_; + NiceMock* mock_upstream_ready_cb_; std::vector test_clients_; }; @@ -136,7 +140,9 @@ class ConnPoolImplForTest : public Event::TestUsingSimulatedTime, public FixedHt class Http1ConnPoolImplTest : public testing::Test { public: Http1ConnPoolImplTest() - : conn_pool_(std::make_unique(dispatcher_, cluster_, random_)) {} + : upstream_ready_cb_(new NiceMock(&dispatcher_)), + conn_pool_(std::make_unique(dispatcher_, cluster_, random_, + upstream_ready_cb_)) {} ~Http1ConnPoolImplTest() override { EXPECT_EQ("", TestUtility::nonZeroedGauges(cluster_->stats_store_.gauges())); @@ -145,6 +151,7 @@ class Http1ConnPoolImplTest : public testing::Test { NiceMock random_; NiceMock dispatcher_; std::shared_ptr cluster_{new NiceMock()}; + NiceMock* upstream_ready_cb_; std::unique_ptr conn_pool_; NiceMock runtime_; }; @@ -290,13 +297,16 @@ TEST_F(Http1ConnPoolImplTest, VerifyAlpnFallback) { })); cluster_->transport_socket_matcher_ = std::make_unique>(std::move(factory)); + new NiceMock(&dispatcher_); // Recreate the conn pool so that the host re-evaluates the transport socket match, arriving at // our test transport socket factory. - conn_pool_ = std::make_unique(dispatcher_, cluster_, random_); + conn_pool_ = + std::make_unique(dispatcher_, cluster_, random_, upstream_ready_cb_); NiceMock outer_decoder; ConnPoolCallbacks callbacks; conn_pool_->expectClientCreate(Protocol::Http11); + Http::ConnectionPool::Cancellable* handle = conn_pool_->newStream(outer_decoder, callbacks); EXPECT_NE(nullptr, handle); @@ -646,7 +656,6 @@ TEST_F(Http1ConnPoolImplTest, MaxConnections) { inner_decoder->decodeHeaders(std::move(response_headers), true); conn_pool_->expectAndRunUpstreamReady(); - conn_pool_->expectEnableUpstreamReady(); EXPECT_TRUE( callbacks2.outer_encoder_ ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) @@ -716,7 +725,6 @@ TEST_F(Http1ConnPoolImplTest, ConnectionCloseWithoutHeader) { EXPECT_CALL(callbacks2.pool_ready_, ready()); conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected); - conn_pool_->expectEnableUpstreamReady(); EXPECT_TRUE( callbacks2.outer_encoder_ ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) @@ -978,9 +986,7 @@ TEST_F(Http1ConnPoolImplTest, ConcurrentConnections) { r3.startRequest(); EXPECT_EQ(3U, cluster_->stats_.upstream_rq_total_.value()); - conn_pool_->expectEnableUpstreamReady(); r2.completeResponse(false); - conn_pool_->expectEnableUpstreamReady(); r3.completeResponse(false); // Disconnect both clients. @@ -1125,6 +1131,56 @@ TEST_F(Http1ConnPoolImplTest, PendingRequestIsConsideredActive) { EXPECT_EQ(1U, cluster_->stats_.upstream_cx_destroy_local_.value()); } +class ConnPoolImplNoDestructForTest : public ConnPoolImplForTest { +public: + ConnPoolImplNoDestructForTest(Event::MockDispatcher& dispatcher, + Upstream::ClusterInfoConstSharedPtr cluster, + Random::RandomGenerator& random_generator, + NiceMock* upstream_ready_cb) + : ConnPoolImplForTest(dispatcher, cluster, random_generator, upstream_ready_cb) {} + + ~ConnPoolImplNoDestructForTest() override { destructAllConnections(); } +}; + +// Regression test for connection pool use after free when dispatcher runs posted callback +// conn_pool->onUpstreamReady() after connection pool is destroyed. +TEST_F(Http1ConnPoolImplTest, UseAfterFreeConnPool) { + // Recreate conn pool without destructor checks. + new NiceMock(&dispatcher_); + conn_pool_ = std::make_unique(dispatcher_, cluster_, random_, + upstream_ready_cb_); + + InSequence s; + + NiceMock outer_decoder; + ConnPoolCallbacks callbacks; + conn_pool_->expectClientCreate(); + Http::ConnectionPool::Cancellable* handle = conn_pool_->newStream(outer_decoder, callbacks); + EXPECT_NE(nullptr, handle); + + NiceMock request_encoder; + ResponseDecoder* inner_decoder; + EXPECT_CALL(*conn_pool_->test_clients_[0].codec_, newStream(_)) + .WillOnce(DoAll(SaveArgAddress(&inner_decoder), ReturnRef(request_encoder))); + EXPECT_CALL(callbacks.pool_ready_, ready()); + EXPECT_CALL(*conn_pool_->test_clients_[0].connect_timer_, disableTimer()); + conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected); + + EXPECT_TRUE( + callbacks.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); + + EXPECT_CALL(*conn_pool_, onClientDestroy()); + dispatcher_.deferredDelete(std::move(conn_pool_)); + inner_decoder->decodeHeaders( + ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, true); + dispatcher_.clearDeferredDeleteList(); + + // TODO(asraa): Clean this leak. + delete upstream_ready_cb_; +} + } // namespace } // namespace Http1 } // namespace Http From 715df9566dee4e5c7dfc278daebc116272ea80d4 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Mon, 30 Nov 2020 16:23:20 -0500 Subject: [PATCH 02/17] Revert "fix" This reverts commit b48aee6b30c918a7320942c65afbbaec6b80b69e. Signed-off-by: Asra Ali --- source/common/http/conn_pool_base.cc | 3 +- source/common/http/conn_pool_base.h | 4 -- source/common/http/http1/conn_pool.cc | 5 +- test/common/http/http1/conn_pool_test.cc | 78 ++++-------------------- 4 files changed, 13 insertions(+), 77 deletions(-) diff --git a/source/common/http/conn_pool_base.cc b/source/common/http/conn_pool_base.cc index 3ffa8fb9bb45..19c057a53edb 100644 --- a/source/common/http/conn_pool_base.cc +++ b/source/common/http/conn_pool_base.cc @@ -55,8 +55,7 @@ HttpConnPoolImplBase::HttpConnPoolImplBase( : Envoy::ConnectionPool::ConnPoolImplBase( host, priority, dispatcher, options, wrapTransportSocketOptions(transport_socket_options, protocols), state), - random_generator_(random_generator), - upstream_ready_cb_(dispatcher_.createSchedulableCallback([this]() { onUpstreamReady(); })) { + random_generator_(random_generator) { ASSERT(!protocols.empty()); } diff --git a/source/common/http/conn_pool_base.h b/source/common/http/conn_pool_base.h index 7968bee68ebe..152dc6df86d5 100644 --- a/source/common/http/conn_pool_base.h +++ b/source/common/http/conn_pool_base.h @@ -80,14 +80,10 @@ class HttpConnPoolImplBase : public Envoy::ConnectionPool::ConnPoolImplBase, virtual CodecClientPtr createCodecClient(Upstream::Host::CreateConnectionData& data) PURE; Random::RandomGenerator& randomGenerator() { return random_generator_; } - Event::SchedulableCallback* upstreamReadyCallback() { return upstream_ready_cb_.get(); } protected: friend class ActiveClient; Random::RandomGenerator& random_generator_; - - // onUpstreamReady callback only used by HTTP/1.1 implementation. - Event::SchedulableCallbackPtr upstream_ready_cb_; }; // An implementation of Envoy::ConnectionPool::ActiveClient for HTTP/1.1 and HTTP/2 diff --git a/source/common/http/http1/conn_pool.cc b/source/common/http/http1/conn_pool.cc index a6afd859be09..cc69ab976c0e 100644 --- a/source/common/http/http1/conn_pool.cc +++ b/source/common/http/http1/conn_pool.cc @@ -76,10 +76,7 @@ void ActiveClient::StreamWrapper::onDecodeComplete() { parent_.codec_client_->close(); } else { auto* pool = &parent_.parent(); - if (pool->hasPendingStreams()) { - // SchedulableCallback guarantees callback will only be scheduled if not already enabled. - pool->upstreamReadyCallback()->scheduleCallbackCurrentIteration(); - } + pool->dispatcher().post([pool]() -> void { pool->onUpstreamReady(); }); parent_.stream_wrapper_.reset(); pool->checkForDrained(); diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index 1e20cd7b7999..b86e9c426a31 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -52,8 +52,7 @@ class ConnPoolImplForTest : public Event::TestUsingSimulatedTime, public FixedHt public: ConnPoolImplForTest(Event::MockDispatcher& dispatcher, Upstream::ClusterInfoConstSharedPtr cluster, - Random::RandomGenerator& random_generator, - NiceMock* upstream_ready_cb) + Random::RandomGenerator& random_generator) : FixedHttpConnPoolImpl( Upstream::makeTestHost(cluster, "tcp://127.0.0.1:9000", dispatcher.timeSource()), Upstream::ResourcePriority::Default, dispatcher, nullptr, nullptr, random_generator, @@ -63,8 +62,7 @@ class ConnPoolImplForTest : public Event::TestUsingSimulatedTime, public FixedHt return nullptr; // Not used: createCodecClient overloaded. }, std::vector{Protocol::Http11}), - api_(Api::createApiForTest()), mock_dispatcher_(dispatcher), - mock_upstream_ready_cb_(upstream_ready_cb) {} + api_(Api::createApiForTest()), mock_dispatcher_(dispatcher) {} ~ConnPoolImplForTest() override { EXPECT_EQ(0U, ready_clients_.size()); @@ -120,17 +118,15 @@ class ConnPoolImplForTest : public Event::TestUsingSimulatedTime, public FixedHt } void expectEnableUpstreamReady() { - EXPECT_CALL(*mock_upstream_ready_cb_, scheduleCallbackCurrentIteration()) - .Times(1) - .RetiresOnSaturation(); + EXPECT_CALL(mock_dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb_)); } - void expectAndRunUpstreamReady() { mock_upstream_ready_cb_->invokeCallback(); } + void expectAndRunUpstreamReady() { post_cb_(); } Upstream::ClusterConnectivityState state_; Api::ApiPtr api_; Event::MockDispatcher& mock_dispatcher_; - NiceMock* mock_upstream_ready_cb_; + Event::PostCb post_cb_; std::vector test_clients_; }; @@ -140,9 +136,7 @@ class ConnPoolImplForTest : public Event::TestUsingSimulatedTime, public FixedHt class Http1ConnPoolImplTest : public testing::Test { public: Http1ConnPoolImplTest() - : upstream_ready_cb_(new NiceMock(&dispatcher_)), - conn_pool_(std::make_unique(dispatcher_, cluster_, random_, - upstream_ready_cb_)) {} + : conn_pool_(std::make_unique(dispatcher_, cluster_, random_)) {} ~Http1ConnPoolImplTest() override { EXPECT_EQ("", TestUtility::nonZeroedGauges(cluster_->stats_store_.gauges())); @@ -151,7 +145,6 @@ class Http1ConnPoolImplTest : public testing::Test { NiceMock random_; NiceMock dispatcher_; std::shared_ptr cluster_{new NiceMock()}; - NiceMock* upstream_ready_cb_; std::unique_ptr conn_pool_; NiceMock runtime_; }; @@ -297,16 +290,13 @@ TEST_F(Http1ConnPoolImplTest, VerifyAlpnFallback) { })); cluster_->transport_socket_matcher_ = std::make_unique>(std::move(factory)); - new NiceMock(&dispatcher_); // Recreate the conn pool so that the host re-evaluates the transport socket match, arriving at // our test transport socket factory. - conn_pool_ = - std::make_unique(dispatcher_, cluster_, random_, upstream_ready_cb_); + conn_pool_ = std::make_unique(dispatcher_, cluster_, random_); NiceMock outer_decoder; ConnPoolCallbacks callbacks; conn_pool_->expectClientCreate(Protocol::Http11); - Http::ConnectionPool::Cancellable* handle = conn_pool_->newStream(outer_decoder, callbacks); EXPECT_NE(nullptr, handle); @@ -656,6 +646,7 @@ TEST_F(Http1ConnPoolImplTest, MaxConnections) { inner_decoder->decodeHeaders(std::move(response_headers), true); conn_pool_->expectAndRunUpstreamReady(); + conn_pool_->expectEnableUpstreamReady(); EXPECT_TRUE( callbacks2.outer_encoder_ ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) @@ -725,6 +716,7 @@ TEST_F(Http1ConnPoolImplTest, ConnectionCloseWithoutHeader) { EXPECT_CALL(callbacks2.pool_ready_, ready()); conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected); + conn_pool_->expectEnableUpstreamReady(); EXPECT_TRUE( callbacks2.outer_encoder_ ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) @@ -986,7 +978,9 @@ TEST_F(Http1ConnPoolImplTest, ConcurrentConnections) { r3.startRequest(); EXPECT_EQ(3U, cluster_->stats_.upstream_rq_total_.value()); + conn_pool_->expectEnableUpstreamReady(); r2.completeResponse(false); + conn_pool_->expectEnableUpstreamReady(); r3.completeResponse(false); // Disconnect both clients. @@ -1131,56 +1125,6 @@ TEST_F(Http1ConnPoolImplTest, PendingRequestIsConsideredActive) { EXPECT_EQ(1U, cluster_->stats_.upstream_cx_destroy_local_.value()); } -class ConnPoolImplNoDestructForTest : public ConnPoolImplForTest { -public: - ConnPoolImplNoDestructForTest(Event::MockDispatcher& dispatcher, - Upstream::ClusterInfoConstSharedPtr cluster, - Random::RandomGenerator& random_generator, - NiceMock* upstream_ready_cb) - : ConnPoolImplForTest(dispatcher, cluster, random_generator, upstream_ready_cb) {} - - ~ConnPoolImplNoDestructForTest() override { destructAllConnections(); } -}; - -// Regression test for connection pool use after free when dispatcher runs posted callback -// conn_pool->onUpstreamReady() after connection pool is destroyed. -TEST_F(Http1ConnPoolImplTest, UseAfterFreeConnPool) { - // Recreate conn pool without destructor checks. - new NiceMock(&dispatcher_); - conn_pool_ = std::make_unique(dispatcher_, cluster_, random_, - upstream_ready_cb_); - - InSequence s; - - NiceMock outer_decoder; - ConnPoolCallbacks callbacks; - conn_pool_->expectClientCreate(); - Http::ConnectionPool::Cancellable* handle = conn_pool_->newStream(outer_decoder, callbacks); - EXPECT_NE(nullptr, handle); - - NiceMock request_encoder; - ResponseDecoder* inner_decoder; - EXPECT_CALL(*conn_pool_->test_clients_[0].codec_, newStream(_)) - .WillOnce(DoAll(SaveArgAddress(&inner_decoder), ReturnRef(request_encoder))); - EXPECT_CALL(callbacks.pool_ready_, ready()); - EXPECT_CALL(*conn_pool_->test_clients_[0].connect_timer_, disableTimer()); - conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected); - - EXPECT_TRUE( - callbacks.outer_encoder_ - ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) - .ok()); - - EXPECT_CALL(*conn_pool_, onClientDestroy()); - dispatcher_.deferredDelete(std::move(conn_pool_)); - inner_decoder->decodeHeaders( - ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, true); - dispatcher_.clearDeferredDeleteList(); - - // TODO(asraa): Clean this leak. - delete upstream_ready_cb_; -} - } // namespace } // namespace Http1 } // namespace Http From e0338ad4d3d17add2eab1eba9c4e15cb4f259323 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Tue, 1 Dec 2020 08:51:00 -0500 Subject: [PATCH 03/17] update fix Signed-off-by: Asra Ali --- source/common/conn_pool/conn_pool_base.cc | 13 +++- source/common/conn_pool/conn_pool_base.h | 7 +- source/common/http/http1/conn_pool.cc | 2 +- test/common/http/http1/conn_pool_test.cc | 82 ++++++++++++++++++++--- test/common/http/http2/conn_pool_test.cc | 3 + test/common/http/mixed_conn_pool_test.cc | 4 +- 6 files changed, 96 insertions(+), 15 deletions(-) diff --git a/source/common/conn_pool/conn_pool_base.cc b/source/common/conn_pool/conn_pool_base.cc index daec20bc40fc..5dcf0053783e 100644 --- a/source/common/conn_pool/conn_pool_base.cc +++ b/source/common/conn_pool/conn_pool_base.cc @@ -15,7 +15,11 @@ ConnPoolImplBase::ConnPoolImplBase( const Network::TransportSocketOptionsSharedPtr& transport_socket_options, Upstream::ClusterConnectivityState& state) : state_(state), host_(host), priority_(priority), dispatcher_(dispatcher), - socket_options_(options), transport_socket_options_(transport_socket_options) {} + socket_options_(options), transport_socket_options_(transport_socket_options), + upstream_ready_cb_(dispatcher_.createSchedulableCallback([this]() { + upstream_ready_enabled_ = false; + onUpstreamReady(); + })) {} ConnPoolImplBase::~ConnPoolImplBase() { ASSERT(ready_clients_.empty()); @@ -214,6 +218,13 @@ bool ConnPoolImplBase::maybePrefetch(float global_prefetch_ratio) { return tryCreateNewConnection(global_prefetch_ratio); } +void ConnPoolImplBase::scheduleOnUpstreamReady() { + if (!pending_streams_.empty() && !upstream_ready_enabled_) { + upstream_ready_enabled_ = true; + upstream_ready_cb_->scheduleCallbackCurrentIteration(); + } +} + void ConnPoolImplBase::onUpstreamReady() { while (!pending_streams_.empty() && !ready_clients_.empty()) { ActiveClientPtr& client = ready_clients_.front(); diff --git a/source/common/conn_pool/conn_pool_base.h b/source/common/conn_pool/conn_pool_base.h index 3cc2c2d08953..98429dc3a5b5 100644 --- a/source/common/conn_pool/conn_pool_base.h +++ b/source/common/conn_pool/conn_pool_base.h @@ -154,7 +154,8 @@ class ConnPoolImplBase : protected Logger::Loggable { Network::ConnectionEvent event); // See if the drain process has started and/or completed. void checkForDrained(); - void onUpstreamReady(); + // Schedule onUpstreamReady. + void scheduleOnUpstreamReady(); ConnectionPool::Cancellable* newStream(AttachContext& context); // Called if this pool is likely to be picked soon, to determine if it's worth // prefetching a connection. @@ -241,6 +242,10 @@ class ConnPoolImplBase : protected Logger::Loggable { // Clients that are not ready to handle additional streams because they are CONNECTING. std::list connecting_clients_; + void onUpstreamReady(); + Event::SchedulableCallbackPtr upstream_ready_cb_; + bool upstream_ready_enabled_{false}; + private: std::list pending_streams_; diff --git a/source/common/http/http1/conn_pool.cc b/source/common/http/http1/conn_pool.cc index cc69ab976c0e..61060533cb5c 100644 --- a/source/common/http/http1/conn_pool.cc +++ b/source/common/http/http1/conn_pool.cc @@ -76,7 +76,7 @@ void ActiveClient::StreamWrapper::onDecodeComplete() { parent_.codec_client_->close(); } else { auto* pool = &parent_.parent(); - pool->dispatcher().post([pool]() -> void { pool->onUpstreamReady(); }); + pool->scheduleOnUpstreamReady(); parent_.stream_wrapper_.reset(); pool->checkForDrained(); diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index b86e9c426a31..ee5a59dd548c 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -52,7 +52,8 @@ class ConnPoolImplForTest : public Event::TestUsingSimulatedTime, public FixedHt public: ConnPoolImplForTest(Event::MockDispatcher& dispatcher, Upstream::ClusterInfoConstSharedPtr cluster, - Random::RandomGenerator& random_generator) + Random::RandomGenerator& random_generator, + NiceMock* upstream_ready_cb) : FixedHttpConnPoolImpl( Upstream::makeTestHost(cluster, "tcp://127.0.0.1:9000", dispatcher.timeSource()), Upstream::ResourcePriority::Default, dispatcher, nullptr, nullptr, random_generator, @@ -62,7 +63,8 @@ class ConnPoolImplForTest : public Event::TestUsingSimulatedTime, public FixedHt return nullptr; // Not used: createCodecClient overloaded. }, std::vector{Protocol::Http11}), - api_(Api::createApiForTest()), mock_dispatcher_(dispatcher) {} + api_(Api::createApiForTest()), mock_dispatcher_(dispatcher), + mock_upstream_ready_cb_(upstream_ready_cb) {} ~ConnPoolImplForTest() override { EXPECT_EQ(0U, ready_clients_.size()); @@ -118,15 +120,22 @@ class ConnPoolImplForTest : public Event::TestUsingSimulatedTime, public FixedHt } void expectEnableUpstreamReady() { - EXPECT_CALL(mock_dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb_)); + EXPECT_FALSE(upstream_ready_enabled_); + EXPECT_CALL(*mock_upstream_ready_cb_, scheduleCallbackCurrentIteration()) + .Times(1) + .RetiresOnSaturation(); } - void expectAndRunUpstreamReady() { post_cb_(); } + void expectAndRunUpstreamReady() { + EXPECT_TRUE(upstream_ready_enabled_); + mock_upstream_ready_cb_->invokeCallback(); + EXPECT_FALSE(upstream_ready_enabled_); + } Upstream::ClusterConnectivityState state_; Api::ApiPtr api_; Event::MockDispatcher& mock_dispatcher_; - Event::PostCb post_cb_; + NiceMock* mock_upstream_ready_cb_; std::vector test_clients_; }; @@ -136,7 +145,9 @@ class ConnPoolImplForTest : public Event::TestUsingSimulatedTime, public FixedHt class Http1ConnPoolImplTest : public testing::Test { public: Http1ConnPoolImplTest() - : conn_pool_(std::make_unique(dispatcher_, cluster_, random_)) {} + : upstream_ready_cb_(new NiceMock(&dispatcher_)), + conn_pool_(std::make_unique(dispatcher_, cluster_, random_, + upstream_ready_cb_)) {} ~Http1ConnPoolImplTest() override { EXPECT_EQ("", TestUtility::nonZeroedGauges(cluster_->stats_store_.gauges())); @@ -145,6 +156,7 @@ class Http1ConnPoolImplTest : public testing::Test { NiceMock random_; NiceMock dispatcher_; std::shared_ptr cluster_{new NiceMock()}; + NiceMock* upstream_ready_cb_; std::unique_ptr conn_pool_; NiceMock runtime_; }; @@ -293,7 +305,9 @@ TEST_F(Http1ConnPoolImplTest, VerifyAlpnFallback) { // Recreate the conn pool so that the host re-evaluates the transport socket match, arriving at // our test transport socket factory. - conn_pool_ = std::make_unique(dispatcher_, cluster_, random_); + new NiceMock(&dispatcher_); + conn_pool_ = + std::make_unique(dispatcher_, cluster_, random_, upstream_ready_cb_); NiceMock outer_decoder; ConnPoolCallbacks callbacks; conn_pool_->expectClientCreate(Protocol::Http11); @@ -646,7 +660,6 @@ TEST_F(Http1ConnPoolImplTest, MaxConnections) { inner_decoder->decodeHeaders(std::move(response_headers), true); conn_pool_->expectAndRunUpstreamReady(); - conn_pool_->expectEnableUpstreamReady(); EXPECT_TRUE( callbacks2.outer_encoder_ ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) @@ -716,7 +729,6 @@ TEST_F(Http1ConnPoolImplTest, ConnectionCloseWithoutHeader) { EXPECT_CALL(callbacks2.pool_ready_, ready()); conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected); - conn_pool_->expectEnableUpstreamReady(); EXPECT_TRUE( callbacks2.outer_encoder_ ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) @@ -978,9 +990,7 @@ TEST_F(Http1ConnPoolImplTest, ConcurrentConnections) { r3.startRequest(); EXPECT_EQ(3U, cluster_->stats_.upstream_rq_total_.value()); - conn_pool_->expectEnableUpstreamReady(); r2.completeResponse(false); - conn_pool_->expectEnableUpstreamReady(); r3.completeResponse(false); // Disconnect both clients. @@ -1125,6 +1135,56 @@ TEST_F(Http1ConnPoolImplTest, PendingRequestIsConsideredActive) { EXPECT_EQ(1U, cluster_->stats_.upstream_cx_destroy_local_.value()); } +class ConnPoolImplNoDestructForTest : public ConnPoolImplForTest { +public: + ConnPoolImplNoDestructForTest(Event::MockDispatcher& dispatcher, + Upstream::ClusterInfoConstSharedPtr cluster, + Random::RandomGenerator& random_generator, + NiceMock* upstream_ready_cb) + : ConnPoolImplForTest(dispatcher, cluster, random_generator, upstream_ready_cb) {} + + ~ConnPoolImplNoDestructForTest() override { destructAllConnections(); } +}; + +// Regression test for use after free when dispatcher executes onUpstreamReady after connection pool +// is destroyed. +TEST_F(Http1ConnPoolImplTest, CbAfterConnPoolDestroyed) { + // Recreate conn pool without destructor checks. + new NiceMock(&dispatcher_); + conn_pool_ = std::make_unique(dispatcher_, cluster_, random_, + upstream_ready_cb_); + + InSequence s; + + NiceMock outer_decoder; + ConnPoolCallbacks callbacks; + conn_pool_->expectClientCreate(); + Http::ConnectionPool::Cancellable* handle = conn_pool_->newStream(outer_decoder, callbacks); + EXPECT_NE(nullptr, handle); + + NiceMock request_encoder; + ResponseDecoder* inner_decoder; + EXPECT_CALL(*conn_pool_->test_clients_[0].codec_, newStream(_)) + .WillOnce(DoAll(SaveArgAddress(&inner_decoder), ReturnRef(request_encoder))); + EXPECT_CALL(callbacks.pool_ready_, ready()); + EXPECT_CALL(*conn_pool_->test_clients_[0].connect_timer_, disableTimer()); + conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected); + + EXPECT_TRUE( + callbacks.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); + + EXPECT_CALL(*conn_pool_, onClientDestroy()); + dispatcher_.deferredDelete(std::move(conn_pool_)); + + inner_decoder->decodeHeaders( + ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, true); + + // Remove connection pool. + dispatcher_.clearDeferredDeleteList(); +} + } // namespace } // namespace Http1 } // namespace Http diff --git a/test/common/http/http2/conn_pool_test.cc b/test/common/http/http2/conn_pool_test.cc index d15076d5785c..4787a4d188c2 100644 --- a/test/common/http/http2/conn_pool_test.cc +++ b/test/common/http/http2/conn_pool_test.cc @@ -74,6 +74,7 @@ class Http2ConnPoolImplTest : public Event::TestUsingSimulatedTime, public testi Http2ConnPoolImplTest() : api_(Api::createApiForTest(stats_store_)), + upstream_ready_cb_(new NiceMock(&dispatcher_)), pool_(std::make_unique(dispatcher_, random_, host_, Upstream::ResourcePriority::Default, nullptr, nullptr, state_)) { @@ -212,6 +213,7 @@ class Http2ConnPoolImplTest : public Event::TestUsingSimulatedTime, public testi NiceMock dispatcher_; std::shared_ptr cluster_{new NiceMock()}; Upstream::HostSharedPtr host_{Upstream::makeTestHost(cluster_, "tcp://127.0.0.1:80", simTime())}; + NiceMock* upstream_ready_cb_; std::unique_ptr pool_; std::vector test_clients_; NiceMock runtime_; @@ -337,6 +339,7 @@ TEST_F(Http2ConnPoolImplTest, VerifyAlpnFallback) { // Recreate the conn pool so that the host re-evaluates the transport socket match, arriving at // our test transport socket factory. host_ = Upstream::makeTestHost(cluster_, "tcp://127.0.0.1:80", simTime()); + new NiceMock(&dispatcher_); pool_ = std::make_unique( dispatcher_, random_, host_, Upstream::ResourcePriority::Default, nullptr, nullptr, state_); diff --git a/test/common/http/mixed_conn_pool_test.cc b/test/common/http/mixed_conn_pool_test.cc index b826d2e29851..9e0e00a88550 100644 --- a/test/common/http/mixed_conn_pool_test.cc +++ b/test/common/http/mixed_conn_pool_test.cc @@ -39,7 +39,8 @@ class ConnPoolImplForTest : public Event::TestUsingSimulatedTime, public HttpCon class MixedConnPoolImplTest : public testing::Test { public: MixedConnPoolImplTest() - : conn_pool_(std::make_unique(dispatcher_, state_, random_, cluster_)) {} + : upstream_ready_cb_(new NiceMock(&dispatcher_)), + conn_pool_(std::make_unique(dispatcher_, state_, random_, cluster_)) {} ~MixedConnPoolImplTest() override { EXPECT_EQ("", TestUtility::nonZeroedGauges(cluster_->stats_store_.gauges())); @@ -48,6 +49,7 @@ class MixedConnPoolImplTest : public testing::Test { Upstream::ClusterConnectivityState state_; NiceMock dispatcher_; std::shared_ptr cluster_{new NiceMock()}; + NiceMock* upstream_ready_cb_; std::unique_ptr conn_pool_; NiceMock runtime_; NiceMock random_; From 56e2f13e72512a00cae3a27589ba761f05b50178 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Tue, 1 Dec 2020 15:13:48 -0500 Subject: [PATCH 04/17] address comments, tcp test wip Signed-off-by: Asra Ali --- source/common/conn_pool/conn_pool_base.cc | 8 ++------ source/common/conn_pool/conn_pool_base.h | 2 -- source/common/tcp/conn_pool.cc | 4 ++-- test/common/http/http1/conn_pool_test.cc | 13 +++++-------- test/common/tcp/conn_pool_test.cc | 21 ++++----------------- 5 files changed, 13 insertions(+), 35 deletions(-) diff --git a/source/common/conn_pool/conn_pool_base.cc b/source/common/conn_pool/conn_pool_base.cc index 5dcf0053783e..be305e55f361 100644 --- a/source/common/conn_pool/conn_pool_base.cc +++ b/source/common/conn_pool/conn_pool_base.cc @@ -16,10 +16,7 @@ ConnPoolImplBase::ConnPoolImplBase( Upstream::ClusterConnectivityState& state) : state_(state), host_(host), priority_(priority), dispatcher_(dispatcher), socket_options_(options), transport_socket_options_(transport_socket_options), - upstream_ready_cb_(dispatcher_.createSchedulableCallback([this]() { - upstream_ready_enabled_ = false; - onUpstreamReady(); - })) {} + upstream_ready_cb_(dispatcher_.createSchedulableCallback([this]() { onUpstreamReady(); })) {} ConnPoolImplBase::~ConnPoolImplBase() { ASSERT(ready_clients_.empty()); @@ -219,8 +216,7 @@ bool ConnPoolImplBase::maybePrefetch(float global_prefetch_ratio) { } void ConnPoolImplBase::scheduleOnUpstreamReady() { - if (!pending_streams_.empty() && !upstream_ready_enabled_) { - upstream_ready_enabled_ = true; + if (hasPendingStreams()) { upstream_ready_cb_->scheduleCallbackCurrentIteration(); } } diff --git a/source/common/conn_pool/conn_pool_base.h b/source/common/conn_pool/conn_pool_base.h index 98429dc3a5b5..5a135351c7c8 100644 --- a/source/common/conn_pool/conn_pool_base.h +++ b/source/common/conn_pool/conn_pool_base.h @@ -154,7 +154,6 @@ class ConnPoolImplBase : protected Logger::Loggable { Network::ConnectionEvent event); // See if the drain process has started and/or completed. void checkForDrained(); - // Schedule onUpstreamReady. void scheduleOnUpstreamReady(); ConnectionPool::Cancellable* newStream(AttachContext& context); // Called if this pool is likely to be picked soon, to determine if it's worth @@ -244,7 +243,6 @@ class ConnPoolImplBase : protected Logger::Loggable { void onUpstreamReady(); Event::SchedulableCallbackPtr upstream_ready_cb_; - bool upstream_ready_enabled_{false}; private: std::list pending_streams_; diff --git a/source/common/tcp/conn_pool.cc b/source/common/tcp/conn_pool.cc index 5abc86fc959c..d49d0afedce6 100644 --- a/source/common/tcp/conn_pool.cc +++ b/source/common/tcp/conn_pool.cc @@ -47,9 +47,9 @@ ActiveTcpClient::~ActiveTcpClient() { } void ActiveTcpClient::clearCallbacks() { - if (state_ == Envoy::ConnectionPool::ActiveClient::State::BUSY && parent_.hasPendingStreams()) { + if (state_ == Envoy::ConnectionPool::ActiveClient::State::BUSY) { auto* pool = &parent_; - pool->dispatcher().post([pool]() -> void { pool->onUpstreamReady(); }); + pool->scheduleOnUpstreamReady(); } callbacks_ = nullptr; tcp_connection_data_ = nullptr; diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index ee5a59dd548c..c5fc4a62e27c 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -120,22 +120,17 @@ class ConnPoolImplForTest : public Event::TestUsingSimulatedTime, public FixedHt } void expectEnableUpstreamReady() { - EXPECT_FALSE(upstream_ready_enabled_); EXPECT_CALL(*mock_upstream_ready_cb_, scheduleCallbackCurrentIteration()) .Times(1) .RetiresOnSaturation(); } - void expectAndRunUpstreamReady() { - EXPECT_TRUE(upstream_ready_enabled_); - mock_upstream_ready_cb_->invokeCallback(); - EXPECT_FALSE(upstream_ready_enabled_); - } + void expectAndRunUpstreamReady() { mock_upstream_ready_cb_->invokeCallback(); } Upstream::ClusterConnectivityState state_; Api::ApiPtr api_; Event::MockDispatcher& mock_dispatcher_; - NiceMock* mock_upstream_ready_cb_; + Event::MockSchedulableCallback* mock_upstream_ready_cb_; std::vector test_clients_; }; @@ -305,7 +300,9 @@ TEST_F(Http1ConnPoolImplTest, VerifyAlpnFallback) { // Recreate the conn pool so that the host re-evaluates the transport socket match, arriving at // our test transport socket factory. - new NiceMock(&dispatcher_); + EXPECT_CALL(dispatcher_, createSchedulableCallback_(_)) + .WillOnce(Return(upstream_ready_cb_)) + .RetiresOnSaturation(); conn_pool_ = std::make_unique(dispatcher_, cluster_, random_, upstream_ready_cb_); NiceMock outer_decoder; diff --git a/test/common/tcp/conn_pool_test.cc b/test/common/tcp/conn_pool_test.cc index 08e7f9fae2a6..c3c7c95c796a 100644 --- a/test/common/tcp/conn_pool_test.cc +++ b/test/common/tcp/conn_pool_test.cc @@ -227,16 +227,7 @@ ConnPoolBase::ConnPoolBase(Event::MockDispatcher& dispatcher, Upstream::HostShar } void ConnPoolBase::expectEnableUpstreamReady(bool run) { - if (!test_new_connection_pool_) { - dynamic_cast(conn_pool_.get())->expectEnableUpstreamReady(run); - } else { - Event::PostCb& post_cb = dynamic_cast(conn_pool_.get())->post_cb_; - if (run) { - post_cb(); - } else { - EXPECT_CALL(mock_dispatcher_, post(_)).WillOnce(testing::SaveArg<0>(&post_cb)); - } - } + dynamic_cast(conn_pool_.get())->expectEnableUpstreamReady(run); } /** @@ -247,9 +238,7 @@ class TcpConnPoolImplTest : public Event::TestUsingSimulatedTime, public: TcpConnPoolImplTest() : test_new_connection_pool_(GetParam()), - upstream_ready_cb_(test_new_connection_pool_ - ? nullptr - : new NiceMock(&dispatcher_)), + upstream_ready_cb_(new NiceMock(&dispatcher_)), host_(Upstream::makeTestHost(cluster_, "tcp://127.0.0.1:9000", simTime())), conn_pool_(dispatcher_, host_, upstream_ready_cb_, test_new_connection_pool_) {} @@ -275,9 +264,7 @@ class TcpConnPoolImplDestructorTest : public Event::TestUsingSimulatedTime, public: TcpConnPoolImplDestructorTest() : test_new_connection_pool_(GetParam()), - upstream_ready_cb_(test_new_connection_pool_ - ? nullptr - : new NiceMock(&dispatcher_)) { + upstream_ready_cb_(new NiceMock(&dispatcher_)) { host_ = Upstream::makeTestHost(cluster_, "tcp://127.0.0.1:9000", simTime()); if (test_new_connection_pool_) { conn_pool_ = std::make_unique( @@ -784,7 +771,7 @@ TEST_P(TcpConnPoolImplTest, MaxConnections) { // Finishing request 1 will immediately bind to request 2. EXPECT_CALL(conn_pool_, onConnReleasedForTest()); conn_pool_.expectEnableUpstreamReady(false); - EXPECT_CALL(callbacks2.pool_ready_, ready()); + EXPECT_CALL(callbacks2.pool_ready_, ready()); // never gets triggered when reset. callbacks.conn_data_.reset(); conn_pool_.expectEnableUpstreamReady(true); From 906c83f9ccf4eb33494d98c83a2620386a7cec7d Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Wed, 2 Dec 2020 09:06:16 -0500 Subject: [PATCH 05/17] fix h/1 test Signed-off-by: Asra Ali --- source/common/conn_pool/conn_pool_base.cc | 4 +- test/common/http/http1/conn_pool_test.cc | 51 ++++++++++++++++++----- 2 files changed, 42 insertions(+), 13 deletions(-) diff --git a/source/common/conn_pool/conn_pool_base.cc b/source/common/conn_pool/conn_pool_base.cc index be305e55f361..9bd90c190983 100644 --- a/source/common/conn_pool/conn_pool_base.cc +++ b/source/common/conn_pool/conn_pool_base.cc @@ -216,9 +216,7 @@ bool ConnPoolImplBase::maybePrefetch(float global_prefetch_ratio) { } void ConnPoolImplBase::scheduleOnUpstreamReady() { - if (hasPendingStreams()) { - upstream_ready_cb_->scheduleCallbackCurrentIteration(); - } + upstream_ready_cb_->scheduleCallbackCurrentIteration(); } void ConnPoolImplBase::onUpstreamReady() { diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index c5fc4a62e27c..b6858296f611 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -53,7 +53,7 @@ class ConnPoolImplForTest : public Event::TestUsingSimulatedTime, public FixedHt ConnPoolImplForTest(Event::MockDispatcher& dispatcher, Upstream::ClusterInfoConstSharedPtr cluster, Random::RandomGenerator& random_generator, - NiceMock* upstream_ready_cb) + Event::MockSchedulableCallback* upstream_ready_cb) : FixedHttpConnPoolImpl( Upstream::makeTestHost(cluster, "tcp://127.0.0.1:9000", dispatcher.timeSource()), Upstream::ResourcePriority::Default, dispatcher, nullptr, nullptr, random_generator, @@ -657,6 +657,7 @@ TEST_F(Http1ConnPoolImplTest, MaxConnections) { inner_decoder->decodeHeaders(std::move(response_headers), true); conn_pool_->expectAndRunUpstreamReady(); + conn_pool_->expectEnableUpstreamReady(); EXPECT_TRUE( callbacks2.outer_encoder_ ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) @@ -726,6 +727,7 @@ TEST_F(Http1ConnPoolImplTest, ConnectionCloseWithoutHeader) { EXPECT_CALL(callbacks2.pool_ready_, ready()); conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected); + conn_pool_->expectEnableUpstreamReady(); EXPECT_TRUE( callbacks2.outer_encoder_ ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) @@ -987,7 +989,9 @@ TEST_F(Http1ConnPoolImplTest, ConcurrentConnections) { r3.startRequest(); EXPECT_EQ(3U, cluster_->stats_.upstream_rq_total_.value()); + conn_pool_->expectEnableUpstreamReady(); r2.completeResponse(false); + conn_pool_->expectEnableUpstreamReady(); r3.completeResponse(false); // Disconnect both clients. @@ -1132,25 +1136,50 @@ TEST_F(Http1ConnPoolImplTest, PendingRequestIsConsideredActive) { EXPECT_EQ(1U, cluster_->stats_.upstream_cx_destroy_local_.value()); } +// Schedulable callback that can track it's destruction. +class MockDestructSchedulableCallback : public Event::MockSchedulableCallback { +public: + MockDestructSchedulableCallback(Event::MockDispatcher* dispatcher) + : Event::MockSchedulableCallback(dispatcher), destroyed_(false) {} + ~MockDestructSchedulableCallback() override { destroyed_ = true; } + + bool destroyed_{false}; +}; + +// Connection pool for test that destructs all connections when destroyed. class ConnPoolImplNoDestructForTest : public ConnPoolImplForTest { public: ConnPoolImplNoDestructForTest(Event::MockDispatcher& dispatcher, Upstream::ClusterInfoConstSharedPtr cluster, Random::RandomGenerator& random_generator, - NiceMock* upstream_ready_cb) + MockDestructSchedulableCallback* upstream_ready_cb) : ConnPoolImplForTest(dispatcher, cluster, random_generator, upstream_ready_cb) {} ~ConnPoolImplNoDestructForTest() override { destructAllConnections(); } }; +class Http1ConnPoolDestructImplTest : public testing::Test { +public: + Http1ConnPoolDestructImplTest() + : upstream_ready_cb_(new MockDestructSchedulableCallback(&dispatcher_)), + conn_pool_(std::make_unique(dispatcher_, cluster_, random_, + upstream_ready_cb_)) {} + + ~Http1ConnPoolDestructImplTest() override { + EXPECT_EQ("", TestUtility::nonZeroedGauges(cluster_->stats_store_.gauges())); + } + + NiceMock random_; + NiceMock dispatcher_; + std::shared_ptr cluster_{new NiceMock()}; + MockDestructSchedulableCallback* upstream_ready_cb_; + std::unique_ptr conn_pool_; + NiceMock runtime_; +}; + // Regression test for use after free when dispatcher executes onUpstreamReady after connection pool // is destroyed. -TEST_F(Http1ConnPoolImplTest, CbAfterConnPoolDestroyed) { - // Recreate conn pool without destructor checks. - new NiceMock(&dispatcher_); - conn_pool_ = std::make_unique(dispatcher_, cluster_, random_, - upstream_ready_cb_); - +TEST_F(Http1ConnPoolDestructImplTest, CbAfterConnPoolDestroyed) { InSequence s; NiceMock outer_decoder; @@ -1172,14 +1201,16 @@ TEST_F(Http1ConnPoolImplTest, CbAfterConnPoolDestroyed) { ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) .ok()); + conn_pool_->expectEnableUpstreamReady(); EXPECT_CALL(*conn_pool_, onClientDestroy()); dispatcher_.deferredDelete(std::move(conn_pool_)); - inner_decoder->decodeHeaders( ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, true); - // Remove connection pool. + // Delete connection pool and check that scheduled callback onUpstreamReady was destroyed. + EXPECT_FALSE(upstream_ready_cb_->destroyed_); dispatcher_.clearDeferredDeleteList(); + EXPECT_TRUE(upstream_ready_cb_->destroyed_); } } // namespace From cd767c02deb44510d6253d7a7f3f35b024f3c5fb Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Wed, 2 Dec 2020 09:09:09 -0500 Subject: [PATCH 06/17] revert tcp Signed-off-by: Asra Ali --- source/common/tcp/conn_pool.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/tcp/conn_pool.cc b/source/common/tcp/conn_pool.cc index d49d0afedce6..e905b2337711 100644 --- a/source/common/tcp/conn_pool.cc +++ b/source/common/tcp/conn_pool.cc @@ -47,7 +47,7 @@ ActiveTcpClient::~ActiveTcpClient() { } void ActiveTcpClient::clearCallbacks() { - if (state_ == Envoy::ConnectionPool::ActiveClient::State::BUSY) { + if (state_ == Envoy::ConnectionPool::ActiveClient::State::BUSY && parent_.hasPendingStreams()) { auto* pool = &parent_; pool->scheduleOnUpstreamReady(); } From 2c3443954d2ad9e777211ff9dca2d69af3274cf2 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Wed, 2 Dec 2020 11:19:30 -0500 Subject: [PATCH 07/17] fix tcp test Signed-off-by: Asra Ali --- test/common/tcp/conn_pool_test.cc | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/test/common/tcp/conn_pool_test.cc b/test/common/tcp/conn_pool_test.cc index c3c7c95c796a..993c1c757ef3 100644 --- a/test/common/tcp/conn_pool_test.cc +++ b/test/common/tcp/conn_pool_test.cc @@ -227,7 +227,17 @@ ConnPoolBase::ConnPoolBase(Event::MockDispatcher& dispatcher, Upstream::HostShar } void ConnPoolBase::expectEnableUpstreamReady(bool run) { - dynamic_cast(conn_pool_.get())->expectEnableUpstreamReady(run); + if (!test_new_connection_pool_) { + dynamic_cast(conn_pool_.get())->expectEnableUpstreamReady(run); + } else { + if (run) { + mock_upstream_ready_cb_->invokeCallback(); + } else { + EXPECT_CALL(*mock_upstream_ready_cb_, scheduleCallbackCurrentIteration()) + .Times(1) + .RetiresOnSaturation(); + } + } } /** From 14004b0e5d771ea7e7ff2b298c9d558ffe9a2caa Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Wed, 2 Dec 2020 11:20:47 -0500 Subject: [PATCH 08/17] remove extraneous tcp comment Signed-off-by: Asra Ali --- test/common/tcp/conn_pool_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/tcp/conn_pool_test.cc b/test/common/tcp/conn_pool_test.cc index 993c1c757ef3..5fb2510e304a 100644 --- a/test/common/tcp/conn_pool_test.cc +++ b/test/common/tcp/conn_pool_test.cc @@ -781,7 +781,7 @@ TEST_P(TcpConnPoolImplTest, MaxConnections) { // Finishing request 1 will immediately bind to request 2. EXPECT_CALL(conn_pool_, onConnReleasedForTest()); conn_pool_.expectEnableUpstreamReady(false); - EXPECT_CALL(callbacks2.pool_ready_, ready()); // never gets triggered when reset. + EXPECT_CALL(callbacks2.pool_ready_, ready()); callbacks.conn_data_.reset(); conn_pool_.expectEnableUpstreamReady(true); From e75320f41c38cdea90888eb9e529ee2bb89c0ff0 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Wed, 2 Dec 2020 12:07:12 -0500 Subject: [PATCH 09/17] make mock strict Signed-off-by: Asra Ali --- test/common/http/http1/conn_pool_test.cc | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index b6858296f611..731069d21473 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -140,7 +140,7 @@ class ConnPoolImplForTest : public Event::TestUsingSimulatedTime, public FixedHt class Http1ConnPoolImplTest : public testing::Test { public: Http1ConnPoolImplTest() - : upstream_ready_cb_(new NiceMock(&dispatcher_)), + : upstream_ready_cb_(new Event::MockSchedulableCallback(&dispatcher_)), conn_pool_(std::make_unique(dispatcher_, cluster_, random_, upstream_ready_cb_)) {} @@ -151,7 +151,7 @@ class Http1ConnPoolImplTest : public testing::Test { NiceMock random_; NiceMock dispatcher_; std::shared_ptr cluster_{new NiceMock()}; - NiceMock* upstream_ready_cb_; + Event::MockSchedulableCallback* upstream_ready_cb_; std::unique_ptr conn_pool_; NiceMock runtime_; }; @@ -250,14 +250,17 @@ TEST_F(Http1ConnPoolImplTest, DrainConnections) { ActiveTestRequest r2(*this, 1, ActiveTestRequest::Type::CreateConnection); r2.startRequest(); + conn_pool_->expectEnableUpstreamReady(); r1.completeResponse(false); // This will destroy the ready client and set requests remaining to 1 on the busy client. conn_pool_->drainConnections(); EXPECT_CALL(*conn_pool_, onClientDestroy()); dispatcher_.clearDeferredDeleteList(); + conn_pool_->expectAndRunUpstreamReady(); // This will destroy the busy client when the response finishes. + conn_pool_->expectEnableUpstreamReady(); r2.completeResponse(false); EXPECT_CALL(*conn_pool_, onClientDestroy()); dispatcher_.clearDeferredDeleteList(); @@ -274,9 +277,11 @@ TEST_F(Http1ConnPoolImplTest, VerifyTimingStats) { ActiveTestRequest r1(*this, 0, ActiveTestRequest::Type::CreateConnection); r1.startRequest(); + conn_pool_->expectEnableUpstreamReady(); r1.completeResponse(false); EXPECT_CALL(*conn_pool_, onClientDestroy()); + conn_pool_->expectAndRunUpstreamReady(); conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::RemoteClose); dispatcher_.clearDeferredDeleteList(); } @@ -377,15 +382,18 @@ TEST_F(Http1ConnPoolImplTest, MultipleRequestAndResponse) { // Request 1 should kick off a new connection. ActiveTestRequest r1(*this, 0, ActiveTestRequest::Type::CreateConnection); r1.startRequest(); + conn_pool_->expectEnableUpstreamReady(); r1.completeResponse(false); // Request 2 should not. ActiveTestRequest r2(*this, 0, ActiveTestRequest::Type::Immediate); r2.startRequest(); + conn_pool_->expectEnableUpstreamReady(); r2.completeResponse(true); // Cause the connection to go away. EXPECT_CALL(*conn_pool_, onClientDestroy()); + conn_pool_->expectAndRunUpstreamReady(); conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::RemoteClose); dispatcher_.clearDeferredDeleteList(); } @@ -953,6 +961,7 @@ TEST_F(Http1ConnPoolImplTest, MaxRequestsPerConnection) { EXPECT_CALL(callbacks.pool_ready_, ready()); conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected); + conn_pool_->expectEnableUpstreamReady(); EXPECT_TRUE( callbacks.outer_encoder_ ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) @@ -1016,8 +1025,10 @@ TEST_F(Http1ConnPoolImplTest, DrainCallback) { r2.handle_->cancel(Envoy::ConnectionPool::CancelPolicy::Default); EXPECT_EQ(1U, cluster_->stats_.upstream_rq_total_.value()); + conn_pool_->expectEnableUpstreamReady(); EXPECT_CALL(drained, ready()); r1.startRequest(); + r1.completeResponse(false); EXPECT_CALL(*conn_pool_, onClientDestroy()); @@ -1101,15 +1112,18 @@ TEST_F(Http1ConnPoolImplTest, ActiveRequestHasActiveConnectionsTrue) { EXPECT_TRUE(conn_pool_->hasActiveConnections()); // cleanup + conn_pool_->expectEnableUpstreamReady(); r1.completeResponse(false); conn_pool_->drainConnections(); EXPECT_CALL(*conn_pool_, onClientDestroy()); dispatcher_.clearDeferredDeleteList(); + conn_pool_->expectAndRunUpstreamReady(); } TEST_F(Http1ConnPoolImplTest, ResponseCompletedConnectionReadyNoActiveConnections) { ActiveTestRequest r1(*this, 0, ActiveTestRequest::Type::CreateConnection); r1.startRequest(); + conn_pool_->expectEnableUpstreamReady(); r1.completeResponse(false); EXPECT_FALSE(conn_pool_->hasActiveConnections()); @@ -1117,6 +1131,7 @@ TEST_F(Http1ConnPoolImplTest, ResponseCompletedConnectionReadyNoActiveConnection conn_pool_->drainConnections(); EXPECT_CALL(*conn_pool_, onClientDestroy()); dispatcher_.clearDeferredDeleteList(); + conn_pool_->expectAndRunUpstreamReady(); } TEST_F(Http1ConnPoolImplTest, PendingRequestIsConsideredActive) { From 5f1d06274158cb808a77db33c4e4256e65fa561d Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Wed, 2 Dec 2020 16:00:04 -0500 Subject: [PATCH 10/17] fix docker test Signed-off-by: Asra Ali --- test/common/http/http1/conn_pool_test.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index 731069d21473..1085166fcaac 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -305,9 +305,8 @@ TEST_F(Http1ConnPoolImplTest, VerifyAlpnFallback) { // Recreate the conn pool so that the host re-evaluates the transport socket match, arriving at // our test transport socket factory. - EXPECT_CALL(dispatcher_, createSchedulableCallback_(_)) - .WillOnce(Return(upstream_ready_cb_)) - .RetiresOnSaturation(); + // Recreate this to refresh expectation that the callback is scheduled and saved. + new Event::MockSchedulableCallback(&dispatcher_); conn_pool_ = std::make_unique(dispatcher_, cluster_, random_, upstream_ready_cb_); NiceMock outer_decoder; From 65405497df1048a7948390ed720933d515a1560a Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Thu, 3 Dec 2020 09:50:33 -0500 Subject: [PATCH 11/17] fix asan Signed-off-by: Asra Ali --- test/common/conn_pool/conn_pool_base_test.cc | 4 +++- test/common/http/http1/conn_pool_test.cc | 9 ++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/test/common/conn_pool/conn_pool_base_test.cc b/test/common/conn_pool/conn_pool_base_test.cc index cc79fbea4572..1ecd11f73f2a 100644 --- a/test/common/conn_pool/conn_pool_base_test.cc +++ b/test/common/conn_pool/conn_pool_base_test.cc @@ -52,7 +52,8 @@ class TestConnPoolImplBase : public ConnPoolImplBase { class ConnPoolImplBaseTest : public testing::Test { public: ConnPoolImplBaseTest() - : pool_(host_, Upstream::ResourcePriority::Default, dispatcher_, nullptr, nullptr, state_) { + : upstream_ready_cb_(new NiceMock(&dispatcher_)), + pool_(host_, Upstream::ResourcePriority::Default, dispatcher_, nullptr, nullptr, state_) { // Default connections to 1024 because the tests shouldn't be relying on the // connection resource limit for most tests. cluster_->resetResourceManager(1024, 1024, 1024, 1, 1); @@ -80,6 +81,7 @@ class ConnPoolImplBaseTest : public testing::Test { new NiceMock()}; std::shared_ptr cluster_{new NiceMock()}; NiceMock dispatcher_; + NiceMock* upstream_ready_cb_; Upstream::HostSharedPtr host_{ Upstream::makeTestHost(cluster_, "tcp://127.0.0.1:80", dispatcher_.timeSource())}; TestConnPoolImplBase pool_; diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index 1085166fcaac..3aec305ec74b 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -1154,10 +1154,10 @@ TEST_F(Http1ConnPoolImplTest, PendingRequestIsConsideredActive) { class MockDestructSchedulableCallback : public Event::MockSchedulableCallback { public: MockDestructSchedulableCallback(Event::MockDispatcher* dispatcher) - : Event::MockSchedulableCallback(dispatcher), destroyed_(false) {} - ~MockDestructSchedulableCallback() override { destroyed_ = true; } + : Event::MockSchedulableCallback(dispatcher) {} + MOCK_METHOD0(Die, void()); - bool destroyed_{false}; + ~MockDestructSchedulableCallback() override { Die(); } }; // Connection pool for test that destructs all connections when destroyed. @@ -1222,9 +1222,8 @@ TEST_F(Http1ConnPoolDestructImplTest, CbAfterConnPoolDestroyed) { ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, true); // Delete connection pool and check that scheduled callback onUpstreamReady was destroyed. - EXPECT_FALSE(upstream_ready_cb_->destroyed_); + EXPECT_CALL(*upstream_ready_cb_, Die()); dispatcher_.clearDeferredDeleteList(); - EXPECT_TRUE(upstream_ready_cb_->destroyed_); } } // namespace From d0b08e38f727ac92e742338c94b324e829ceeeed Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Fri, 4 Dec 2020 14:16:42 -0500 Subject: [PATCH 12/17] avoid recursing in clearDeferredDeleteList Signed-off-by: Asra Ali --- test/common/http/http1/conn_pool_test.cc | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index 3aec305ec74b..3de239ebfb2d 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -1169,7 +1169,7 @@ class ConnPoolImplNoDestructForTest : public ConnPoolImplForTest { MockDestructSchedulableCallback* upstream_ready_cb) : ConnPoolImplForTest(dispatcher, cluster, random_generator, upstream_ready_cb) {} - ~ConnPoolImplNoDestructForTest() override { destructAllConnections(); } + ~ConnPoolImplNoDestructForTest() override {} }; class Http1ConnPoolDestructImplTest : public testing::Test { @@ -1216,13 +1216,30 @@ TEST_F(Http1ConnPoolDestructImplTest, CbAfterConnPoolDestroyed) { .ok()); conn_pool_->expectEnableUpstreamReady(); - EXPECT_CALL(*conn_pool_, onClientDestroy()); - dispatcher_.deferredDelete(std::move(conn_pool_)); + // Schedules the onUpstreamReady callback. inner_decoder->decodeHeaders( ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, true); + // Delete the connection pool. + EXPECT_CALL(*conn_pool_, onClientDestroy()); + conn_pool_->destructAllConnections(); + // Delete connection pool and check that scheduled callback onUpstreamReady was destroyed. + dispatcher_.deferredDelete(std::move(conn_pool_)); EXPECT_CALL(*upstream_ready_cb_, Die()); + + // When the dispatcher removes the connection pool, another call to clearDeferredDeleteList() + // cocurs in ~HttpConnPoolImplBase. Avoid recursion. + bool deferring_delete = false; + ON_CALL(dispatcher_, clearDeferredDeleteList()) + .WillByDefault(Invoke([this, &deferring_delete]() -> void { + if (deferring_delete) { + return; + } + deferring_delete = true; + dispatcher_.to_delete_.clear(); + deferring_delete = false; + })); dispatcher_.clearDeferredDeleteList(); } From 2b76b5280d40446f1e66606fdbb68c928f3402d3 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Fri, 4 Dec 2020 14:36:30 -0500 Subject: [PATCH 13/17] fix comment Signed-off-by: Asra Ali --- test/common/http/http1/conn_pool_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index 3de239ebfb2d..06bd2a72da80 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -1160,7 +1160,7 @@ class MockDestructSchedulableCallback : public Event::MockSchedulableCallback { ~MockDestructSchedulableCallback() override { Die(); } }; -// Connection pool for test that destructs all connections when destroyed. +// Connection pool for test that holds on to a schedulable callback that has destructor detection. class ConnPoolImplNoDestructForTest : public ConnPoolImplForTest { public: ConnPoolImplNoDestructForTest(Event::MockDispatcher& dispatcher, From fac261a61ba73494d428f0ef20d5f4a2ea7553fd Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Fri, 4 Dec 2020 16:04:14 -0500 Subject: [PATCH 14/17] fix spelling Signed-off-by: Asra Ali --- test/common/http/http1/conn_pool_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index 06bd2a72da80..83abc927e97c 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -1229,7 +1229,7 @@ TEST_F(Http1ConnPoolDestructImplTest, CbAfterConnPoolDestroyed) { EXPECT_CALL(*upstream_ready_cb_, Die()); // When the dispatcher removes the connection pool, another call to clearDeferredDeleteList() - // cocurs in ~HttpConnPoolImplBase. Avoid recursion. + // occurs in ~HttpConnPoolImplBase. Avoid recursion. bool deferring_delete = false; ON_CALL(dispatcher_, clearDeferredDeleteList()) .WillByDefault(Invoke([this, &deferring_delete]() -> void { From 43e3cadde43d26e98c2ecffb0115ef70e077e129 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Fri, 4 Dec 2020 21:16:50 -0500 Subject: [PATCH 15/17] add default keyword Signed-off-by: Asra Ali --- test/common/http/http1/conn_pool_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index 83abc927e97c..7a4bcaaeeb23 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -1169,7 +1169,7 @@ class ConnPoolImplNoDestructForTest : public ConnPoolImplForTest { MockDestructSchedulableCallback* upstream_ready_cb) : ConnPoolImplForTest(dispatcher, cluster, random_generator, upstream_ready_cb) {} - ~ConnPoolImplNoDestructForTest() override {} + ~ConnPoolImplNoDestructForTest() override {} = default; }; class Http1ConnPoolDestructImplTest : public testing::Test { From 2247fae5501579d1c2b18bbc54970d8ef4bb778e Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Sat, 5 Dec 2020 15:12:21 -0500 Subject: [PATCH 16/17] simplify Signed-off-by: Asra Ali --- test/common/http/http1/conn_pool_test.cc | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index 7a4bcaaeeb23..9b6937abe3b4 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -1160,24 +1160,12 @@ class MockDestructSchedulableCallback : public Event::MockSchedulableCallback { ~MockDestructSchedulableCallback() override { Die(); } }; -// Connection pool for test that holds on to a schedulable callback that has destructor detection. -class ConnPoolImplNoDestructForTest : public ConnPoolImplForTest { -public: - ConnPoolImplNoDestructForTest(Event::MockDispatcher& dispatcher, - Upstream::ClusterInfoConstSharedPtr cluster, - Random::RandomGenerator& random_generator, - MockDestructSchedulableCallback* upstream_ready_cb) - : ConnPoolImplForTest(dispatcher, cluster, random_generator, upstream_ready_cb) {} - - ~ConnPoolImplNoDestructForTest() override {} = default; -}; - class Http1ConnPoolDestructImplTest : public testing::Test { public: Http1ConnPoolDestructImplTest() : upstream_ready_cb_(new MockDestructSchedulableCallback(&dispatcher_)), - conn_pool_(std::make_unique(dispatcher_, cluster_, random_, - upstream_ready_cb_)) {} + conn_pool_(std::make_unique(dispatcher_, cluster_, random_, + upstream_ready_cb_)) {} ~Http1ConnPoolDestructImplTest() override { EXPECT_EQ("", TestUtility::nonZeroedGauges(cluster_->stats_store_.gauges())); From b6304a44f8a97ea8a5db147d9ba623f9782cad00 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Mon, 7 Dec 2020 13:45:31 -0500 Subject: [PATCH 17/17] make private Signed-off-by: Asra Ali --- source/common/conn_pool/conn_pool_base.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/common/conn_pool/conn_pool_base.h b/source/common/conn_pool/conn_pool_base.h index 5a135351c7c8..084681fc4cd9 100644 --- a/source/common/conn_pool/conn_pool_base.h +++ b/source/common/conn_pool/conn_pool_base.h @@ -241,9 +241,6 @@ class ConnPoolImplBase : protected Logger::Loggable { // Clients that are not ready to handle additional streams because they are CONNECTING. std::list connecting_clients_; - void onUpstreamReady(); - Event::SchedulableCallbackPtr upstream_ready_cb_; - private: std::list pending_streams_; @@ -253,6 +250,9 @@ class ConnPoolImplBase : protected Logger::Loggable { // The number of streams that can be immediately dispatched // if all CONNECTING connections become connected. uint32_t connecting_stream_capacity_{0}; + + void onUpstreamReady(); + Event::SchedulableCallbackPtr upstream_ready_cb_; }; } // namespace ConnectionPool