diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 42b49a1cd751f..2b3796b3ff6ee 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -1439,8 +1439,9 @@ ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::connPool( have_transport_socket_options ? context->upstreamTransportSocketOptions() : nullptr, parent_.parent_.time_source_, parent_.cluster_manager_state_); - pool->addIdleCallback( - [this, host, priority, hash_key]() { httpConnPoolIsIdle(host, priority, hash_key); }); + pool->addIdleCallback([&parent = parent_, host, priority, hash_key]() { + parent.httpConnPoolIsIdle(host, priority, hash_key); + }); return pool; }); @@ -1452,15 +1453,15 @@ ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::connPool( } } -void ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::httpConnPoolIsIdle( +void ClusterManagerImpl::ThreadLocalClusterManagerImpl::httpConnPoolIsIdle( HostConstSharedPtr host, ResourcePriority priority, const std::vector& hash_key) { - if (parent_.destroying_) { + if (destroying_) { // If the Cluster is being destroyed, this pool will be cleaned up by that // process. return; } - ConnPoolsContainer* container = parent_.getHttpConnPoolsContainer(host); + ConnPoolsContainer* container = getHttpConnPoolsContainer(host); if (container == nullptr) { // This could happen if we have cleaned out the host before iterating through every // connection pool. Handle it by just continuing. @@ -1478,7 +1479,7 @@ void ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::httpConnPo // comment in `ClusterManagerImpl::ThreadLocalClusterManagerImpl::drainConnPools`. if (!container->do_not_delete_ && container->pools_->size() == 0) { ENVOY_LOG(trace, "Pool container empty for host {}, erasing host entry", host); - parent_.host_http_conn_pool_map_.erase( + host_http_conn_pool_map_.erase( host); // NOTE: `container` is erased after this point in the lambda. } } @@ -1533,21 +1534,21 @@ ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::tcpConnPool( parent_.cluster_manager_state_)); ASSERT(inserted); pool_iter->second->addIdleCallback( - [this, host, hash_key]() { tcpConnPoolIsIdle(host, hash_key); }); + [&parent = parent_, host, hash_key]() { parent.tcpConnPoolIsIdle(host, hash_key); }); } return pool_iter->second.get(); } -void ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::tcpConnPoolIsIdle( +void ClusterManagerImpl::ThreadLocalClusterManagerImpl::tcpConnPoolIsIdle( HostConstSharedPtr host, const std::vector& hash_key) { - if (parent_.destroying_) { + if (destroying_) { // If the Cluster is being destroyed, this pool will be cleaned up by that process. return; } - auto it = parent_.host_tcp_conn_pool_map_.find(host); - if (it != parent_.host_tcp_conn_pool_map_.end()) { + auto it = host_tcp_conn_pool_map_.find(host); + if (it != host_tcp_conn_pool_map_.end()) { TcpConnPoolsContainer& container = it->second; auto erase_iter = container.pools_.find(hash_key); @@ -1555,13 +1556,13 @@ void ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::tcpConnPoo if (container.draining_ || Runtime::runtimeFeatureEnabled("envoy.reloadable_features.conn_pool_delete_when_idle")) { ENVOY_LOG(trace, "Idle pool, erasing pool for host {}", host); - parent_.thread_local_dispatcher_.deferredDelete(std::move(erase_iter->second)); + thread_local_dispatcher_.deferredDelete(std::move(erase_iter->second)); container.pools_.erase(erase_iter); } } if (container.pools_.empty()) { - parent_.host_tcp_conn_pool_map_.erase( + host_tcp_conn_pool_map_.erase( host); // NOTE: `container` is erased after this point in the lambda. } } diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index 8f66d055f49e5..11cb8c2d0b4cf 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -412,10 +412,6 @@ class ClusterManagerImpl : public ClusterManager, Logger::Loggable& hash_key); - void tcpConnPoolIsIdle(HostConstSharedPtr host, const std::vector& hash_key); - // Upstream::ThreadLocalCluster const PrioritySet& prioritySet() override { return priority_set_; } ClusterInfoConstSharedPtr info() override { return cluster_info_; } @@ -453,6 +449,9 @@ class ClusterManagerImpl : public ClusterManager, Logger::Loggable& hash_key); + void tcpConnPoolIsIdle(HostConstSharedPtr host, const std::vector& hash_key); void removeTcpConn(const HostConstSharedPtr& host, Network::ClientConnection& connection); void removeHosts(const std::string& name, const HostVector& hosts_removed); void updateClusterMembership(const std::string& name, uint32_t priority, diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index a7ace7425ba85..28023040e9be6 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -1594,15 +1594,17 @@ TEST_F(ClusterManagerImplTest, DynamicAddRemove) { EXPECT_EQ(cluster2->info_, cluster_manager_->getThreadLocalCluster("fake_cluster")->info()); EXPECT_EQ(1UL, cluster_manager_->clusters().active_clusters_.size()); Http::ConnectionPool::MockInstance* cp = new Http::ConnectionPool::MockInstance(); + Http::ConnectionPool::Instance::IdleCb idle_cb; EXPECT_CALL(factory_, allocateConnPool_(_, _, _, _, _)).WillOnce(Return(cp)); - EXPECT_CALL(*cp, addIdleCallback(_)); + EXPECT_CALL(*cp, addIdleCallback(_)).WillOnce(SaveArg<0>(&idle_cb)); EXPECT_EQ(cp, HttpPoolDataPeer::getPool(cluster_manager_->getThreadLocalCluster("fake_cluster") ->httpConnPool(ResourcePriority::Default, Http::Protocol::Http11, nullptr))); Tcp::ConnectionPool::MockInstance* cp2 = new Tcp::ConnectionPool::MockInstance(); + Tcp::ConnectionPool::Instance::IdleCb idle_cb2; EXPECT_CALL(factory_, allocateTcpConnPool_(_)).WillOnce(Return(cp2)); - EXPECT_CALL(*cp2, addIdleCallback(_)); + EXPECT_CALL(*cp2, addIdleCallback(_)).WillOnce(SaveArg<0>(&idle_cb2)); EXPECT_EQ(cp2, TcpPoolDataPeer::getPool(cluster_manager_->getThreadLocalCluster("fake_cluster") ->tcpConnPool(ResourcePriority::Default, nullptr))); @@ -1633,6 +1635,9 @@ TEST_F(ClusterManagerImplTest, DynamicAddRemove) { // Remove an unknown cluster. EXPECT_FALSE(cluster_manager_->removeCluster("foo")); + idle_cb(); + idle_cb2(); + checkStats(1 /*added*/, 1 /*modified*/, 1 /*removed*/, 0 /*active*/, 0 /*warming*/); EXPECT_TRUE(Mock::VerifyAndClearExpectations(cluster1.get()));