Skip to content

Commit

Permalink
Fix overflow in timeout logic
Browse files Browse the repository at this point in the history
Also use steady clock consistently.

Prior to this change, the test would fail under ASAN:

bazel test --config=asan --test_output=errors //sdk/test/metrics:meter_provider_sdk_test
INFO: Analyzed target //sdk/test/metrics:meter_provider_sdk_test (0 packages loaded, 0 targets configured).
FAIL: //sdk/test/metrics:meter_provider_sdk_test (see /private/var/tmp/_bazel_punya/e3bd968ba61238cdeb1a5537fc3dbf7d/execroot/_main/bazel-out/darwin_arm64-fastbuild-asan/testlogs/sdk/test/metrics/meter_provider_sdk_test/test.log)
INFO: From Testing //sdk/test/metrics:meter_provider_sdk_test:
==================== Test output for //sdk/test/metrics:meter_provider_sdk_test:
Running main() from gmock_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from MeterProvider
[ RUN      ] MeterProvider.GetMeter
[Warning] File: sdk/src/metrics/meter_provider.cc:65 [MeterProvider::GetMeter] Library name is empty.
[Warning] File: sdk/src/metrics/meter_provider.cc:65 [MeterProvider::GetMeter] Library name is empty.
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/__chrono/duration.h:102:59: runtime error: signed integer overflow: 9221646818050376183 * 1000 cannot be represented in type '_Ct' (aka 'long long')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/__chrono/duration.h:102:59 in 
================================================================================
INFO: Found 1 test target...
Target //sdk/test/metrics:meter_provider_sdk_test up-to-date:
  bazel-bin/sdk/test/metrics/meter_provider_sdk_test
INFO: Elapsed time: 2.251s, Critical Path: 2.13s
INFO: 5 processes: 5 darwin-sandbox.
INFO: Build completed, 1 test FAILED, 5 total actions
//sdk/test/metrics:meter_provider_sdk_test                               FAILED in 0.6s
  /private/var/tmp/_bazel_punya/e3bd968ba61238cdeb1a5537fc3dbf7d/execroot/_main/bazel-out/darwin_arm64-fastbuild-asan/testlogs/sdk/test/metrics/meter_provider_sdk_test/test.log

Executed 1 out of 1 test: 1 fails locally.

Fix overflow in periodic_exporting_metric_reader
  • Loading branch information
punya committed Sep 1, 2024
1 parent a920898 commit 200f6ab
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 23 deletions.
2 changes: 1 addition & 1 deletion sdk/src/metrics/export/periodic_exporting_metric_reader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ bool PeriodicExportingMetricReader::OnForceFlush(std::chrono::microseconds timeo
// - If original `timeout` is `zero`, use that in exporter::forceflush
// - Else if remaining `timeout_steady` more than zero, use that in exporter::forceflush
// - Else don't invoke exporter::forceflush ( as remaining time is zero or less)
if (timeout <= std::chrono::steady_clock::duration::zero())
if (timeout <= std::chrono::milliseconds::duration::zero())
{
result =
exporter_->ForceFlush(std::chrono::duration_cast<std::chrono::microseconds>(timeout));
Expand Down
34 changes: 12 additions & 22 deletions sdk/src/metrics/meter_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -168,46 +168,36 @@ bool MeterContext::ForceFlush(std::chrono::microseconds timeout) noexcept
bool result = true;
// Simultaneous flush not allowed.
const std::lock_guard<opentelemetry::common::SpinLockMutex> locked(forceflush_lock_);
// Convert to nanos to prevent overflow
auto timeout_ns = (std::chrono::nanoseconds::max)();
if (std::chrono::duration_cast<std::chrono::microseconds>(timeout_ns) > timeout)
{
timeout_ns = std::chrono::duration_cast<std::chrono::nanoseconds>(timeout);
}

auto current_time = std::chrono::system_clock::now();
std::chrono::system_clock::time_point expire_time;
auto overflow_checker = (std::chrono::system_clock::time_point::max)();

// check if the expected expire time doesn't overflow.
if (overflow_checker - current_time > timeout_ns)
auto time_remaining = (std::chrono::steady_clock::duration::max)();
if (std::chrono::duration_cast<std::chrono::microseconds>(time_remaining) > timeout)
{
expire_time =
current_time + std::chrono::duration_cast<std::chrono::system_clock::duration>(timeout_ns);
time_remaining = timeout;
}
else

auto current_time = std::chrono::steady_clock::now();
auto expire_time = (std::chrono::steady_clock::time_point::max)();
if (expire_time - current_time > time_remaining)
{
// overflow happens, reset expire time to max.
expire_time = overflow_checker;
expire_time = current_time + time_remaining;
}

for (auto &collector : collectors_)
{
if (!std::static_pointer_cast<MetricCollector>(collector)->ForceFlush(
std::chrono::duration_cast<std::chrono::microseconds>(timeout_ns)))
std::chrono::duration_cast<std::chrono::microseconds>(time_remaining)))
{
result = false;
}

current_time = std::chrono::system_clock::now();

current_time = std::chrono::steady_clock::now();
if (expire_time >= current_time)
{
timeout_ns = std::chrono::duration_cast<std::chrono::nanoseconds>(expire_time - current_time);
time_remaining = expire_time - current_time;
}
else
{
timeout_ns = std::chrono::nanoseconds::zero();
time_remaining = std::chrono::steady_clock::duration::zero();
}
}
if (!result)
Expand Down

0 comments on commit 200f6ab

Please sign in to comment.