From c45dff4c9ff6b38b5d7fe471de1cae35e4ab9a7a Mon Sep 17 00:00:00 2001
From: George Arama <50641385+gearama@users.noreply.github.com>
Date: Thu, 8 Jun 2023 11:47:08 -0700
Subject: [PATCH] Fix for the flaky curl test (#4670)

* Fix for the flaky curl test

* Larry's comment

* clang format applied

* updated comment

* clang-format-11 -i
---
 .../curl/curl_connection_pool_private.hpp     |  2 +
 .../test/ut/azure_libcurl_core_main_test.cpp  | 55 +++++++++++++++++--
 2 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/sdk/core/azure-core/src/http/curl/curl_connection_pool_private.hpp b/sdk/core/azure-core/src/http/curl/curl_connection_pool_private.hpp
index fff264bb2..382fc6e20 100644
--- a/sdk/core/azure-core/src/http/curl/curl_connection_pool_private.hpp
+++ b/sdk/core/azure-core/src/http/curl/curl_connection_pool_private.hpp
@@ -29,6 +29,7 @@ namespace Azure { namespace Core { namespace Test {
   class CurlConnectionPool_connectionPoolTest_Test;
   class CurlConnectionPool_uniquePort_Test;
   class CurlConnectionPool_connectionClose_Test;
+  class SdkWithLibcurl_globalCleanUp_Test;
 }}} // namespace Azure::Core::Test
 #endif
 
@@ -47,6 +48,7 @@ namespace Azure { namespace Core { namespace Http { namespace _detail {
     friend class Azure::Core::Test::CurlConnectionPool_connectionPoolTest_Test;
     friend class Azure::Core::Test::CurlConnectionPool_uniquePort_Test;
     friend class Azure::Core::Test::CurlConnectionPool_connectionClose_Test;
+    friend class Azure::Core::Test::SdkWithLibcurl_globalCleanUp_Test;
 #endif
 
   public:
diff --git a/sdk/core/azure-core/test/ut/azure_libcurl_core_main_test.cpp b/sdk/core/azure-core/test/ut/azure_libcurl_core_main_test.cpp
index 2f50d25f1..b4879aaa2 100644
--- a/sdk/core/azure-core/test/ut/azure_libcurl_core_main_test.cpp
+++ b/sdk/core/azure-core/test/ut/azure_libcurl_core_main_test.cpp
@@ -26,6 +26,7 @@
 
 #include <csignal>
 #include <cstdlib>
+#include <thread>
 
 #include <gtest/gtest.h>
 
@@ -34,7 +35,12 @@ namespace Azure { namespace Core { namespace Test {
   {
     Azure::Core::Http::Request req(
         Azure::Core::Http::HttpMethod::Get, Azure::Core::Url("https://httpbin.org/get"));
+    using std::chrono::duration;
+    using std::chrono::duration_cast;
+    using std::chrono::high_resolution_clock;
+    using std::chrono::milliseconds;
 
+    auto t1 = high_resolution_clock::now();
     {
       // Creating a new connection with default options
       Azure::Core::Http::CurlTransportOptions options;
@@ -52,12 +58,51 @@ namespace Azure { namespace Core { namespace Test {
       EXPECT_TRUE(session->IsEOF());
       EXPECT_TRUE(session->m_keepAlive);
       EXPECT_FALSE(session->m_connectionUpgraded);
+      t1 = high_resolution_clock::now();
+    }
+    // here the session is destroyed and the connection is moved to the pool
+    // the same destructor also makes a call to start the cleanup thread
+    // which will sleep for DefaultCleanerIntervalMilliseconds then loop through the connections
+    // in the pool and check for the ones that are expired(DefaultConnectionExpiredMilliseconds) and
+    // remove them which will be the case here if we wait long enough.
+    // without the calculations below test is flaky due to the
+    // fact that tests in the CI pipeline might take longer than 90 sec to execute thus the cleanup
+    // thread strikes. to have this test be predictable we need to be aware of when we attempt to
+    // read pool size, also we should let things run to completion and then check the pool size thus
+    // the sleep below plus another second to let the for loop in the cleanup thread do its thing.
+
+    auto t2 = high_resolution_clock::now();
+
+    // Getting number of milliseconds as a double.
+    duration<double, std::milli> ms_double = t2 - t1;
+    if (ms_double < duration<double, std::milli>(
+            Azure::Core::Http::_detail::DefaultCleanerIntervalMilliseconds))
+    {
+      // if the destructor execution took less than the cleanup thread sleep the size should be 1
+      EXPECT_EQ(
+          Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex
+              .size(),
+          1);
+      // let the thread cleanup thread hit
+      std::this_thread::sleep_for(
+          std::chrono::milliseconds(
+              Azure::Core::Http::_detail::DefaultCleanerIntervalMilliseconds + 1000)
+          - ms_double);
+      // Check that after the connection is gone and cleaned up, the pool is empty
+      EXPECT_EQ(
+          Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex
+              .size(),
+          0);
+    }
+    else
+    {
+      // we got back from the destructor and thread creation after the cleanup thread hit thus it
+      // will be empty
+      EXPECT_EQ(
+          Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex
+              .size(),
+          0);
     }
-    // Check that after the connection is gone, it is moved back to the pool
-    EXPECT_EQ(
-        Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex
-            .size(),
-        1);
   }
 }}} // namespace Azure::Core::Test