Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 4 additions & 3 deletions ext/src/http/client/curl/http_operation_curl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<float> 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<float> dis(0.8f, 1.2f);

// The initial retry attempt will occur after initialBackoff * random(0.8, 1.2)
auto backoff = retry_policy_.initial_backoff;
Expand Down
31 changes: 31 additions & 0 deletions ext/test/http/curl_http_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Loading