Skip to content

Commit aa35a40

Browse files
committed
Merge remote-tracking branch 'origin/main' into s4406
Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> # Conflicts: # ext/test/http/curl_http_test.cc
2 parents f7f7e77 + 489cd82 commit aa35a40

5 files changed

Lines changed: 173 additions & 43 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,9 @@ Increment the:
9494
return when a retry is scheduled
9595
[#4404](https://github.com/open-telemetry/opentelemetry-cpp/issues/4404)
9696

97+
* [BUG] Keep the curl retry deadline stable within each attempt
98+
[#4452](https://github.com/open-telemetry/opentelemetry-cpp/pull/4452)
99+
97100
## [1.29.0] 2026-09-13
98101

99102
* [RELEASE] Bump main branch to 1.29.0-dev (#4259)

‎examples/grpc/client.cc‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
#include "opentelemetry/nostd/shared_ptr.h"
2121
#include "opentelemetry/semconv/incubating/rpc_attributes.h"
2222
#include "opentelemetry/semconv/network_attributes.h"
23-
#include "opentelemetry/trace/propagation/http_trace_context.h"
2423
#include "opentelemetry/trace/span.h"
2524
#include "opentelemetry/trace/span_metadata.h"
2625
#include "opentelemetry/trace/span_startoptions.h"
@@ -116,10 +115,6 @@ void RunClient(uint16_t port)
116115
int main(int argc, char **argv)
117116
{
118117
InitTracer();
119-
// set global propagator
120-
context::propagation::GlobalTextMapPropagator::SetGlobalPropagator(
121-
opentelemetry::nostd::shared_ptr<context::propagation::TextMapPropagator>(
122-
new propagation::HttpTraceContext()));
123118
constexpr uint16_t default_port = 8800;
124119
uint16_t port{default_port};
125120
if (argc > 1)

‎ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -294,8 +294,8 @@ class HttpOperation
294294
void Abort();
295295

296296
/**
297-
* Perform curl message, this function only can be called in the polling thread and it can only
298-
* be called when got a CURLMSG_DONE.
297+
* Process a completed curl message. This is called directly by synchronous requests or from the
298+
* polling thread after receiving CURLMSG_DONE.
299299
*
300300
* @param code CURLcode
301301
* @return true if the request was re-armed for a retry, false if the operation was cleaned up
@@ -330,6 +330,8 @@ class HttpOperation
330330

331331
const char *GetCurlErrorMessage(CURLcode code);
332332

333+
std::chrono::system_clock::time_point CalculateNextRetryTime();
334+
333335
std::atomic<bool> is_aborted_{false}; // Set to 'true' when async callback is aborted
334336
std::atomic<bool> is_finished_{false}; // Set to 'true' when async callback is finished.
335337
std::atomic<bool> is_cleaned_{false}; // Set to 'true' when async callback is cleaned.
@@ -363,7 +365,7 @@ class HttpOperation
363365
const RetryPolicy retry_policy_;
364366
decltype(RetryPolicy::max_attempts) retry_attempts_;
365367
std::chrono::system_clock::time_point last_attempt_time_;
366-
std::chrono::system_clock::time_point retry_after_time_point_{};
368+
std::chrono::system_clock::time_point next_retry_time_point_{};
367369

368370
// Processed response headers and body
369371
// See CURLINFO_RESPONSE_CODE, type is long

‎ext/src/http/client/curl/http_operation_curl.cc‎

Lines changed: 50 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -650,11 +650,16 @@ bool HttpOperation::IsRetryable()
650650

651651
std::chrono::system_clock::time_point HttpOperation::NextRetryTime()
652652
{
653-
if (retry_after_time_point_ != std::chrono::system_clock::time_point{})
653+
if (next_retry_time_point_ != std::chrono::system_clock::time_point{})
654654
{
655-
return retry_after_time_point_;
655+
return next_retry_time_point_;
656656
}
657657

658+
return CalculateNextRetryTime();
659+
}
660+
661+
std::chrono::system_clock::time_point HttpOperation::CalculateNextRetryTime()
662+
{
658663
// One engine per thread. Every HttpClient drives its own background thread, and drawing from
659664
// the engine advances its state, so a shared one is written by all of them at once.
660665
static thread_local std::mt19937 gen{std::random_device{}()};
@@ -1596,9 +1601,9 @@ void HttpOperation::Abort()
15961601
bool HttpOperation::PerformCurlMessage(CURLcode code)
15971602
{
15981603
++retry_attempts_;
1599-
last_attempt_time_ = std::chrono::system_clock::now();
1600-
last_curl_result_ = code;
1601-
retry_after_time_point_ = std::chrono::system_clock::time_point{};
1604+
last_attempt_time_ = std::chrono::system_clock::now();
1605+
last_curl_result_ = code;
1606+
next_retry_time_point_ = std::chrono::system_clock::time_point{};
16021607

16031608
if (code != CURLE_OK)
16041609
{
@@ -1629,34 +1634,15 @@ bool HttpOperation::PerformCurlMessage(CURLcode code)
16291634
curl_easy_getinfo(curl_resource_.easy_handle, CURLINFO_RESPONSE_CODE, &response_code_);
16301635
}
16311636

1632-
// Transform state
1633-
if (GetSessionState() == opentelemetry::ext::http::client::SessionState::Connecting)
1634-
{
1635-
DispatchEvent(opentelemetry::ext::http::client::SessionState::Connected);
1636-
}
1637-
1638-
if (GetSessionState() == opentelemetry::ext::http::client::SessionState::Connected)
1639-
{
1640-
DispatchEvent(opentelemetry::ext::http::client::SessionState::Sending);
1641-
}
1642-
1643-
if (GetSessionState() == opentelemetry::ext::http::client::SessionState::Sending)
1644-
{
1645-
DispatchEvent(opentelemetry::ext::http::client::SessionState::Response);
1646-
}
1647-
1648-
// A server-driven Retry-After may request a retry far beyond the retry
1649-
// policy's max_backoff (e.g. a malicious or misbehaving server returning
1650-
// "Retry-After: 2 years"). Such a session would linger in the pending retry
1651-
// list and block the FIFO drain in doRetrySessions(), so it must be closed
1652-
// rather than honored. Cap the requested delay at max_backoff; if the
1653-
// server-driven retry time still exceeds the maximum expected retry window,
1654-
// the session is closed below and not retried.
1637+
// Establish the deadline before dispatching Response. Event handlers run synchronously and may
1638+
// inspect it from the callback, so calculating it afterwards could expose a different jittered
1639+
// value from the one retained for scheduling this attempt.
16551640
const bool is_retryable = IsRetryable();
16561641
bool retry_after_exceeds_max_delay = false;
16571642

16581643
if (is_retryable)
16591644
{
1645+
bool has_valid_retry_after = false;
16601646
nostd::string_view retry_after;
16611647
if (FindRetryAfterValue(response_headers_, retry_after))
16621648
{
@@ -1667,23 +1653,55 @@ bool HttpOperation::PerformCurlMessage(CURLcode code)
16671653

16681654
if (parsed_delay || parsed_date)
16691655
{
1656+
has_valid_retry_after = true;
16701657
// Reuse the attempt timestamp instead of calling now() again, so the
16711658
// retry-after delay is measured from the attempt that produced this
16721659
// response and we avoid an extra system_clock syscall on the hot path.
1673-
retry_after_time_point_ = parsed_delay
1674-
? (last_attempt_time_ + delay)
1675-
: ((date > last_attempt_time_) ? date : last_attempt_time_);
1660+
next_retry_time_point_ = parsed_delay
1661+
? (last_attempt_time_ + delay)
1662+
: ((date > last_attempt_time_) ? date : last_attempt_time_);
16761663

16771664
const auto max_retry_time =
16781665
last_attempt_time_ +
16791666
std::chrono::duration_cast<std::chrono::milliseconds>(retry_policy_.max_backoff);
1680-
if (retry_after_time_point_ > max_retry_time)
1667+
if (next_retry_time_point_ > max_retry_time)
16811668
{
16821669
retry_after_exceeds_max_delay = true;
16831670
}
16841671
}
16851672
}
16861673

1674+
if (!has_valid_retry_after)
1675+
{
1676+
next_retry_time_point_ = CalculateNextRetryTime();
1677+
}
1678+
}
1679+
1680+
// Transform state
1681+
if (GetSessionState() == opentelemetry::ext::http::client::SessionState::Connecting)
1682+
{
1683+
DispatchEvent(opentelemetry::ext::http::client::SessionState::Connected);
1684+
}
1685+
1686+
if (GetSessionState() == opentelemetry::ext::http::client::SessionState::Connected)
1687+
{
1688+
DispatchEvent(opentelemetry::ext::http::client::SessionState::Sending);
1689+
}
1690+
1691+
if (GetSessionState() == opentelemetry::ext::http::client::SessionState::Sending)
1692+
{
1693+
DispatchEvent(opentelemetry::ext::http::client::SessionState::Response);
1694+
}
1695+
1696+
// A server-driven Retry-After may request a retry far beyond the retry
1697+
// policy's max_backoff (e.g. a malicious or misbehaving server returning
1698+
// "Retry-After: 2 years"). Such a session would linger in the pending retry
1699+
// list and block the FIFO drain in doRetrySessions(), so it must be closed
1700+
// rather than honored. Cap the requested delay at max_backoff; if the
1701+
// server-driven retry time still exceeds the maximum expected retry window,
1702+
// the session is closed below and not retried.
1703+
if (is_retryable)
1704+
{
16871705
if (!retry_after_exceeds_max_delay)
16881706
{
16891707
// Clear any response data received in previous attempt

‎ext/test/http/curl_http_test.cc‎

Lines changed: 115 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -288,6 +288,26 @@ class RetryEventHandler : public CustomEventHandler
288288
}
289289
};
290290

291+
#ifdef ENABLE_OTLP_RETRY_PREVIEW
292+
class RetryDeadlineEventHandler : public RetryEventHandler
293+
{
294+
public:
295+
void OnEvent(http_client::SessionState state, nostd::string_view reason) noexcept override
296+
{
297+
CustomEventHandler::OnEvent(state, reason);
298+
if (state == http_client::SessionState::Response && operation_ != nullptr)
299+
{
300+
++response_event_count_;
301+
retry_time_during_response_ = operation_->NextRetryTime();
302+
}
303+
}
304+
305+
curl::HttpOperation *operation_{nullptr};
306+
int response_event_count_{0};
307+
std::chrono::system_clock::time_point retry_time_during_response_{};
308+
};
309+
#endif // ENABLE_OTLP_RETRY_PREVIEW
310+
291311
// curl_global_init_mem is the only way to reach the failure returns of curl_slist_append and
292312
// curl_multi_init. It must be called before libcurl is initialised: afterwards it returns
293313
// CURLE_OK and changes nothing. The switches are thread local, so arming one from a test cannot
@@ -662,7 +682,6 @@ class ReportedStateHandler : public CustomEventHandler
662682
std::string reason_;
663683
};
664684
} // namespace
665-
666685
class BasicCurlHttpTests : public ::testing::Test, public HTTP_SERVER_NS::HttpRequestCallback
667686
{
668687
protected:
@@ -719,6 +738,7 @@ class BasicCurlHttpTests : public ::testing::Test, public HTTP_SERVER_NS::HttpRe
719738
server_.addHandler("/post/", *this);
720739
server_.addHandler("/retry/", *this);
721740
server_.addHandler("/retry-after/", *this);
741+
server_.addHandler("/retry-after-invalid/", *this);
722742
server_.addHandler("/close/", *this);
723743
server_.start();
724744
is_running_ = true;
@@ -752,12 +772,21 @@ class BasicCurlHttpTests : public ::testing::Test, public HTTP_SERVER_NS::HttpRe
752772
response.body = "{'k1':'v1', 'k2':'v2', 'k3':'v3'}";
753773
response_status = 200;
754774
}
755-
else if (request.uri == "/retry/")
775+
else if (request.uri == "/retry/" || request.uri == "/retry-after/" ||
776+
request.uri == "/retry-after-invalid/")
756777
{
757778
std::unique_lock<std::mutex> lk1(mtx_requests);
758779
received_requests_.push_back(request);
759780
response.headers["Content-Type"] = "text/plain";
760-
response_status = 429;
781+
if (request.uri == "/retry-after/")
782+
{
783+
response.headers["Retry-After"] = "2";
784+
}
785+
else if (request.uri == "/retry-after-invalid/")
786+
{
787+
response.headers["Retry-After"] = "invalid";
788+
}
789+
response_status = 429;
761790
}
762791
else if (request.uri == "/retry-after/")
763792
{
@@ -1160,6 +1189,89 @@ TEST_F(BasicCurlHttpTests, ExponentialBackoffRetry)
11601189
ASSERT_FALSE(operation.IsRetryable());
11611190
}
11621191

1192+
TEST_F(BasicCurlHttpTests, RetryDeadlineIsStableWithinAttempt)
1193+
{
1194+
RetryEventHandler handler;
1195+
http_client::HttpSslOptions no_ssl;
1196+
http_client::Body body;
1197+
http_client::Headers headers;
1198+
http_client::Compression compression = http_client::Compression::kNone;
1199+
http_client::RetryPolicy retry_policy = {4, std::chrono::duration<float>{1.0f},
1200+
std::chrono::duration<float>{5.0f}, 2.0f};
1201+
1202+
curl::HttpOperation operation(http_client::Method::Post, "http://127.0.0.1:19000/retry/", no_ssl,
1203+
&handler, headers, body, compression, false,
1204+
curl::kDefaultHttpConnTimeout, false, false, retry_policy);
1205+
1206+
ASSERT_EQ(CURLE_OK, operation.Send());
1207+
ASSERT_TRUE(operation.IsRetryable());
1208+
1209+
const auto retry_time = operation.NextRetryTime();
1210+
for (int i = 0; i < 8; ++i)
1211+
{
1212+
EXPECT_EQ(retry_time, operation.NextRetryTime());
1213+
}
1214+
}
1215+
1216+
TEST_F(BasicCurlHttpTests, RetryDeadlineIsReadyDuringResponseEvent)
1217+
{
1218+
RetryDeadlineEventHandler handler;
1219+
http_client::HttpSslOptions no_ssl;
1220+
http_client::Body body;
1221+
http_client::Headers headers;
1222+
http_client::Compression compression = http_client::Compression::kNone;
1223+
http_client::RetryPolicy retry_policy = {4, std::chrono::duration<float>{1.0f},
1224+
std::chrono::duration<float>{5.0f}, 2.0f};
1225+
1226+
curl::HttpOperation operation(http_client::Method::Post, "http://127.0.0.1:19000/retry-after/",
1227+
no_ssl, &handler, headers, body, compression, false,
1228+
curl::kDefaultHttpConnTimeout, false, false, retry_policy);
1229+
handler.operation_ = &operation;
1230+
1231+
const auto before_send = std::chrono::system_clock::now();
1232+
ASSERT_EQ(CURLE_OK, operation.Send());
1233+
const auto after_send = std::chrono::system_clock::now();
1234+
1235+
ASSERT_TRUE(operation.IsRetryable());
1236+
ASSERT_EQ(1, handler.response_event_count_);
1237+
EXPECT_EQ(handler.retry_time_during_response_, operation.NextRetryTime());
1238+
EXPECT_GE(handler.retry_time_during_response_.time_since_epoch().count(),
1239+
(before_send + std::chrono::seconds{2}).time_since_epoch().count());
1240+
EXPECT_LE(handler.retry_time_during_response_.time_since_epoch().count(),
1241+
(after_send + std::chrono::seconds{2}).time_since_epoch().count());
1242+
}
1243+
1244+
TEST_F(BasicCurlHttpTests, InvalidRetryAfterUsesStableBackoff)
1245+
{
1246+
RetryDeadlineEventHandler handler;
1247+
http_client::HttpSslOptions no_ssl;
1248+
http_client::Body body;
1249+
http_client::Headers headers;
1250+
http_client::Compression compression = http_client::Compression::kNone;
1251+
http_client::RetryPolicy retry_policy = {4, std::chrono::duration<float>{1.0f},
1252+
std::chrono::duration<float>{5.0f}, 2.0f};
1253+
1254+
curl::HttpOperation operation(
1255+
http_client::Method::Post, "http://127.0.0.1:19000/retry-after-invalid/", no_ssl, &handler,
1256+
headers, body, compression, false, curl::kDefaultHttpConnTimeout, false, false, retry_policy);
1257+
handler.operation_ = &operation;
1258+
1259+
const auto before_send = std::chrono::system_clock::now();
1260+
ASSERT_EQ(CURLE_OK, operation.Send());
1261+
const auto after_send = std::chrono::system_clock::now();
1262+
1263+
ASSERT_TRUE(operation.IsRetryable());
1264+
ASSERT_EQ(1, handler.response_event_count_);
1265+
for (int i = 0; i < 8; ++i)
1266+
{
1267+
EXPECT_EQ(handler.retry_time_during_response_, operation.NextRetryTime());
1268+
}
1269+
EXPECT_GE(handler.retry_time_during_response_.time_since_epoch().count(),
1270+
(before_send + std::chrono::milliseconds{750}).time_since_epoch().count());
1271+
EXPECT_LE(handler.retry_time_during_response_.time_since_epoch().count(),
1272+
(after_send + std::chrono::milliseconds{1250}).time_since_epoch().count());
1273+
}
1274+
11631275
// A Retry-After beyond max_backoff closes the session. The IO loop used to queue it anyway, where
11641276
// it held back later retries and the background thread until the server's time.
11651277
TEST_F(BasicCurlHttpTests, RetryAfterBeyondMaxBackoffIsNotQueued)

0 commit comments

Comments
 (0)