[TEST] Watch the retry from outside the client, not from inside its queue

The two retry cases waited by polling the retry queue through a test peer. That
queue has no lock, because only the background thread is supposed to touch it,
and a peer reading it from the test thread makes that untrue. ThreadSanitizer
says so: two data races, both this read against the background thread's
push_back, and both of them mine rather than the client's.

So the wait moved outside. It waits on what the server received, which is
behind a mutex and a condition variable, and then for long enough that the
answer has been read and the session put back with its wait on it. The wait is
eight seconds and the settling is one, so a request still outstanding at that
point is outstanding because it is queued, and there are seven seconds left for
the case to be wrong in.

The first attempt at this waited for the server to see a second request, on the
grounds that a retry having fired proves the queue works. It does, but it
proves it one step too late: at that moment the client is reading the second
answer rather than holding a queued session, so cancelling caught a transfer in
flight and the mutation check went green. Both cases now fail against a queue
check that only drops entries with no operation, three runs out of three, at
5.40 seconds against a bound of four, and pass in about a second.

ThreadSanitizer reports nothing on the whole binary now, and all 40 cases pass.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>

Author: thc1006

ext/test/http/curl_http_test.cc

@@ -62,13 +62,6 @@ class HttpClientTestPeer
 
   static bool RemoveSessions(HttpClient &client) { return client.doRemoveSessions(); }
 
-#ifdef ENABLE_OTLP_RETRY_PREVIEW
-  static std::size_t RetryQueueSize(const HttpClient &client)
-  {
-    return client.pending_to_retry_sessions_.size();
-  }
-#endif  // ENABLE_OTLP_RETRY_PREVIEW
-
   static CURLM *ExchangeMultiHandle(HttpClient &client, CURLM *replacement)
   {
     CURLM *previous      = client.multi_handle_;
@@ -1799,30 +1792,30 @@ TEST_F(BasicCurlHttpTests, AClientDestroyedWithAPendingRetryDoesNotWaitForIt)
   // round.
   auto handler = std::make_shared<MultiHandleOutcomeHandler>();
 
-  const auto started = std::chrono::steady_clock::now();
+  std::chrono::steady_clock::time_point started{};
   {
     auto client = std::make_shared<http_client::curl::HttpCurlClientFactory>()->Create();
     ASSERT_TRUE(client != nullptr);
-    auto *concrete = static_cast<http_client::curl::HttpClient *>(client.get());
 
     auto session = client->CreateSession("http://127.0.0.1:19000");
     auto request = session->CreateRequest();
     request->SetUri("retry/");
     request->SetMethod(http_client::Method::Post);
-
-    // Long enough that waiting for it would be unmistakable inside the bound below.
     request->SetRetryPolicy(
         {4, std::chrono::duration<float>{8.0f}, std::chrono::duration<float>{16.0f}, 2.0f});
 
     session->SendRequest(handler);
 
-    for (int i = 0;
-         i < 300 && 0 == http_client::curl::HttpClientTestPeer::RetryQueueSize(*concrete); ++i)
-    {
-      std::this_thread::sleep_for(std::chrono::milliseconds(10));
-    }
-    ASSERT_GE(http_client::curl::HttpClientTestPeer::RetryQueueSize(*concrete), 1u)
-        << "the request was never queued for retry, so nothing was tested";
+    ASSERT_TRUE(waitForRequests(30, 1)) << "the request never reached the server";
+    std::this_thread::sleep_for(std::chrono::seconds(1));
+    ASSERT_EQ(0, handler->terminal_.load(std::memory_order_acquire))
+        << "the request had already finished, so nothing was pending";
+
+    started = std::chrono::steady_clock::now();
+
+    // Nothing cancels it. The client simply goes, which is what an exporter shutting down
+    // does, and the entry it leaves behind names an operation holding a promise and an easy
+    // handle.
   }
   const auto elapsed = std::chrono::duration_cast<std::chrono::milliseconds>(
       std::chrono::steady_clock::now() - started);
@@ -1840,34 +1833,37 @@ TEST_F(BasicCurlHttpTests, ACancelledRetryDoesNotHoldTheClientOpen)
 {
   received_requests_.clear();
 
-  const auto started = std::chrono::steady_clock::now();
+  auto handler = std::make_shared<MultiHandleOutcomeHandler>();
+  std::chrono::steady_clock::time_point started{};
   {
     auto client = std::make_shared<http_client::curl::HttpCurlClientFactory>()->Create();
     ASSERT_TRUE(client != nullptr);
-    auto *concrete = static_cast<http_client::curl::HttpClient *>(client.get());
 
     auto session = client->CreateSession("http://127.0.0.1:19000");
     auto request = session->CreateRequest();
     request->SetUri("retry/");
     request->SetMethod(http_client::Method::Post);
 
-    // Long enough that waiting for it would be unmistakable, and long enough that it cannot
-    // come round on its own inside the bound below.
+    // Long enough that waiting it out would be unmistakable, and long enough that it cannot
+    // come round on its own while the case is still setting up.
     request->SetRetryPolicy(
         {4, std::chrono::duration<float>{8.0f}, std::chrono::duration<float>{16.0f}, 2.0f});
 
-    auto handler = std::make_shared<MultiHandleOutcomeHandler>();
     session->SendRequest(handler);
 
-    // The server answers this route with something retryable, so the session goes into the
-    // retry queue with a time on it that has not come.
-    for (int i = 0;
-         i < 300 && 0 == http_client::curl::HttpClientTestPeer::RetryQueueSize(*concrete); ++i)
-    {
-      std::this_thread::sleep_for(std::chrono::milliseconds(10));
-    }
-    ASSERT_GE(http_client::curl::HttpClientTestPeer::RetryQueueSize(*concrete), 1u)
-        << "the request was never queued for retry, so nothing was tested";
+    // Watched from outside the client. The retry queue has no lock, because only the
+    // background thread is supposed to touch it, and a peer that read it from here would make
+    // that untrue: ThreadSanitizer reports exactly that, twice, against a version of this case
+    // that did. So it waits on what the server saw, which is behind a mutex, and then for long
+    // enough that the answer has been read and the session put back with its wait on it. A
+    // second in, that wait still has seven to run, so anything outstanding here is outstanding
+    // because it is queued.
+    ASSERT_TRUE(waitForRequests(30, 1)) << "the request never reached the server";
+    std::this_thread::sleep_for(std::chrono::seconds(1));
+    ASSERT_EQ(0, handler->terminal_.load(std::memory_order_acquire))
+        << "the request had already finished, so nothing was pending";
+
+    started = std::chrono::steady_clock::now();
 
     // Cancelling takes the operation apart and hands its easy handle back, and the entry left
     // behind names an operation that is not going to be retried by anybody.
@@ -1878,7 +1874,7 @@ TEST_F(BasicCurlHttpTests, ACancelledRetryDoesNotHoldTheClientOpen)
       std::chrono::steady_clock::now() - started);
 
   EXPECT_LT(elapsed.count(), 4000)
-      << "destroying the client took " << elapsed.count()
+      << "tearing the client down took " << elapsed.count()
       << " ms, which is it waiting out a retry for an operation that had already finished";
 }
 #endif  // ENABLE_OTLP_RETRY_PREVIEW