diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f1d54a64a..5bca7b4b1c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,9 @@ Increment the: * [BUG] Report one outcome per request when a curl session is cancelled after the response arrives ([#4363](https://github.com/open-telemetry/opentelemetry-cpp/pull/4363)) +* [BUG] Draw the curl retry jitter from a per thread generator instead of one + shared across HTTP client threads + ([#4399](https://github.com/open-telemetry/opentelemetry-cpp/pull/4399)) * [CODE HEALTH] Enable clang-tidy `modernize-deprecated-headers` and replace deprecated C headers (`stdint.h`, `stddef.h`, `stdlib.h`, `string.h`, `stdio.h`, `ctype.h`, `limits.h`, `assert.h`) with their C++ equivalents diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index d6f49e061c..00b451de85 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -618,9 +618,10 @@ std::chrono::system_clock::time_point HttpOperation::NextRetryTime() return retry_after_time_point_; } - static std::random_device rd; - static std::mt19937 gen(rd()); - static std::uniform_real_distribution dis(0.8f, 1.2f); + // One engine per thread. Every HttpClient drives its own background thread, and drawing from + // the engine advances its state, so a shared one is written by all of them at once. + static thread_local std::mt19937 gen{std::random_device{}()}; + std::uniform_real_distribution dis(0.8f, 1.2f); // The initial retry attempt will occur after initialBackoff * random(0.8, 1.2) auto backoff = retry_policy_.initial_backoff; diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index bfd9153743..e1b180e8fe 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -562,6 +562,37 @@ TEST_F(BasicCurlHttpTests, ACancelBeforeTheResponseReportsCancelled) session_manager->FinishAllSessions(); } +// NextRetryTime draws the backoff jitter from an engine that used to be a shared static, so +// two clients retrying at the same time wrote the same std::mt19937. The case passes either +// way, since a data race is not a functional failure. It is here for the sanitizer builds. +TEST_F(BasicCurlHttpTests, RetryJitterIsNotSharedAcrossThreads) +{ + opentelemetry::ext::http::client::HttpSslOptions ssl_options; + opentelemetry::ext::http::client::Headers request_headers; + opentelemetry::ext::http::client::Body request_body; + + http_client::curl::HttpOperation first(http_client::Method::Get, "http://127.0.0.1:19000/", + ssl_options, nullptr, request_headers, request_body); + http_client::curl::HttpOperation second(http_client::Method::Get, "http://127.0.0.1:19000/", + ssl_options, nullptr, request_headers, request_body); + + std::thread drawing_first([&first] { + for (int i = 0; i < 200; ++i) + { + (void)first.NextRetryTime(); + } + }); + std::thread drawing_second([&second] { + for (int i = 0; i < 200; ++i) + { + (void)second.NextRetryTime(); + } + }); + + drawing_first.join(); + drawing_second.join(); +} + TEST_F(BasicCurlHttpTests, SendGetRequestSync) { received_requests_.clear();