tcp_async_client: move the timer to connect and add null check (envoy…

…proxy#32226) --------- Signed-off-by: Boteng Yao <[email protected]>
fredyw · Feb 13, 2024 · 09042c3 · 09042c3
1 parent 32dd294
commit 09042c3
Show file tree

Hide file tree

Showing 3 changed files with 19 additions and 2 deletions.
diff --git a/envoy/tcp/async_tcp_client.h b/envoy/tcp/async_tcp_client.h
@@ -40,7 +40,8 @@ class AsyncTcpClient {
 
   /**
    * Connect to a remote host. Errors or connection events are reported via the
-   * event callback registered via setAsyncTcpClientCallbacks().
+   * event callback registered via setAsyncTcpClientCallbacks(). We need to set the
+   * callbacks again to call connect() after the connection is disconnected.
    */
   virtual bool connect() PURE;
 

diff --git a/source/common/tcp/async_tcp_client_impl.cc b/source/common/tcp/async_tcp_client_impl.cc
@@ -23,7 +23,6 @@ AsyncTcpClientImpl::AsyncTcpClientImpl(Event::Dispatcher& dispatcher,
       cluster_info_(thread_local_cluster_.info()), context_(context),
       connect_timer_(dispatcher.createTimer([this]() { onConnectTimeout(); })),
       enable_half_close_(enable_half_close) {
-  connect_timer_->enableTimer(cluster_info_->connectTimeout());
   cluster_info_->trafficStats()->upstream_cx_active_.inc();
   cluster_info_->trafficStats()->upstream_cx_total_.inc();
 }
@@ -43,6 +42,10 @@ bool AsyncTcpClientImpl::connect() {
   conn_connect_ms_ = std::make_unique<Stats::HistogramCompletableTimespanImpl>(
       cluster_info_->trafficStats()->upstream_cx_connect_ms_, dispatcher_.timeSource());
 
+  if (!connect_timer_) {
+    connect_timer_ = dispatcher_.createTimer([this]() { onConnectTimeout(); });
+  }
+
   connect_timer_->enableTimer(cluster_info_->connectTimeout());
   connection_->setConnectionStats({cluster_info_->trafficStats()->upstream_cx_rx_bytes_total_,
                                    cluster_info_->trafficStats()->upstream_cx_rx_bytes_buffered_,

diff --git a/test/common/tcp/async_tcp_client_impl_test.cc b/test/common/tcp/async_tcp_client_impl_test.cc
@@ -215,6 +215,19 @@ TEST_F(AsyncTcpClientImplTest, TestFailStats) {
                      ->upstream_cx_connect_fail_.value());
 }
 
+TEST_F(AsyncTcpClientImplTest, TestFailWithReconnect) {
+  setUpClient();
+  expectCreateConnection(false);
+  connection_->raiseEvent(Network::ConnectionEvent::RemoteClose);
+  ASSERT_FALSE(client_->connected());
+  connect_timer_ = new NiceMock<Event::MockTimer>(&dispatcher_);
+  EXPECT_EQ(1UL, cluster_manager_.thread_local_cluster_.cluster_.info_->traffic_stats_
+                     ->upstream_cx_connect_fail_.value());
+  // Reconnect should success without the timer failure.
+  client_->setAsyncTcpClientCallbacks(callbacks_);
+  expectCreateConnection(true);
+}
+
 TEST_F(AsyncTcpClientImplTest, TestCxDestroyRemoteClose) {
   setUpClient();
   expectCreateConnection();