Fix data race which is reported by thread sanitizer

Signed-off-by: owent <admin@owent.net>
open-telemetry · Apr 6, 2022 · 403a87e · 403a87e
1 parent 393e298
commit 403a87e
Show file tree

Hide file tree

Showing 4 changed files with 20 additions and 9 deletions.
diff --git a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h
@@ -9,6 +9,7 @@
 #include "opentelemetry/nostd/shared_ptr.h"
 #include "opentelemetry/version.h"
 
+#include <atomic>
 #include <list>
 #include <mutex>
 #include <string>
@@ -171,7 +172,10 @@ class Session : public opentelemetry::ext::http::client::Session
 
   virtual bool FinishSession() noexcept override;
 
-  virtual bool IsSessionActive() noexcept override { return is_session_active_; }
+  virtual bool IsSessionActive() noexcept override
+  {
+    return is_session_active_.load(std::memory_order_acquire);
+  }
 
   void SetId(uint64_t session_id) { session_id_ = session_id; }
 
@@ -207,7 +211,7 @@ class Session : public opentelemetry::ext::http::client::Session
   std::unique_ptr<HttpOperation> curl_operation_;
   uint64_t session_id_;
   HttpClient &http_client_;
-  bool is_session_active_;
+  std::atomic<bool> is_session_active_;
 };
 
 class HttpClientSync : public opentelemetry::ext::http::client::HttpClientSync

diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc
@@ -35,7 +35,7 @@ nostd::shared_ptr<HttpCurlGlobalInitializer> HttpCurlGlobalInitializer::GetInsta
 void Session::SendRequest(
     std::shared_ptr<opentelemetry::ext::http::client::EventHandler> callback) noexcept
 {
-  is_session_active_    = true;
+  is_session_active_.store(true, std::memory_order_release);
   std::string url       = host_ + std::string(http_request_->uri_);
   auto callback_ptr     = callback.get();
   bool reuse_connection = false;
@@ -64,7 +64,7 @@ void Session::SendRequest(
           response->status_code_ = operation.GetResponseCode();
           callback->OnResponse(*response);
         }
-        is_session_active_ = false;
+        is_session_active_.store(false, std::memory_order_release);
       });
 
   if (success)
@@ -74,7 +74,7 @@ void Session::SendRequest(
   else if (callback)
   {
     callback->OnEvent(opentelemetry::ext::http::client::SessionState::CreateFailed, "");
-    is_session_active_ = false;
+    is_session_active_.store(false, std::memory_order_release);
   }
 }
 

diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc
@@ -572,7 +572,7 @@ CURLcode HttpOperation::SendAsync(Session *session, std::function<void(HttpOpera
   async_data_->callback = std::move(callback);
 
   session->GetHttpClient().ScheduleAddSession(session->GetSessionId());
-  return last_curl_result_;
+  return code;
 }
 
 Headers HttpOperation::GetResponseHeaders()

diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc
@@ -358,8 +358,10 @@ TEST_F(BasicCurlHttpTests, SendGetRequestAsync)
       request->SetUri("get/");
 
       handlers[i] = std::make_shared<GetEventHandler>();
-      sessions[i]->SendRequest(handlers[i]);
 
+      // Lock mtx_requests to prevent response, we will check IsSessionActive() in the end
+      std::unique_lock<std::mutex> lock_requests(mtx_requests);
+      sessions[i]->SendRequest(handlers[i]);
       ASSERT_TRUE(sessions[i]->IsSessionActive());
     }
 
@@ -395,8 +397,10 @@ TEST_F(BasicCurlHttpTests, SendGetRequestAsyncTimeout)
     request->SetTimeoutMs(std::chrono::milliseconds(256));
 
     handlers[i] = std::make_shared<GetEventHandler>();
-    sessions[i]->SendRequest(handlers[i]);
 
+    // Lock mtx_requests to prevent response, we will check IsSessionActive() in the end
+    std::unique_lock<std::mutex> lock_requests(mtx_requests);
+    sessions[i]->SendRequest(handlers[i]);
     ASSERT_TRUE(sessions[i]->IsSessionActive());
   }
 
@@ -427,10 +431,13 @@ TEST_F(BasicCurlHttpTests, SendPostRequestAsync)
       auto request = session->CreateRequest();
       request->SetMethod(http_client::Method::Post);
       request->SetUri("post/");
-      session->SendRequest(handler);
 
+      // Lock mtx_requests to prevent response, we will check IsSessionActive() in the end
+      std::unique_lock<std::mutex> lock_requests(mtx_requests);
+      session->SendRequest(handler);
       ASSERT_TRUE(session->IsSessionActive());
     }
+    lock_requests.unlock();
 
     ASSERT_TRUE(waitForRequests(30, batch_count));