From 19f4a24b8aaac4b654e5a155f90873c3704af077 Mon Sep 17 00:00:00 2001 From: Jonathan Nillius Date: Thu, 16 Apr 2026 15:32:46 +0200 Subject: [PATCH 1/3] [SDK] Add destructor to PeriodicExportingMetricReader to fix shutdown race Destroying a PeriodicExportingMetricReader without calling Shutdown() leaves the background worker thread running inside cv_.wait_for() while the mutex and condition variable members are destroyed. This causes a use-after-destroy race detected by ThreadSanitizer. Add an explicit destructor that calls Shutdown() and joins the worker thread before member destruction, matching the pattern already used by BatchSpanProcessor and BatchLogRecordProcessor. Add a DestroyWithoutShutdown test to verify safe destruction without an explicit Shutdown() call. --- .../export/periodic_exporting_metric_reader.h | 2 ++ .../periodic_exporting_metric_reader.cc | 14 +++++++++++ .../periodic_exporting_metric_reader_test.cc | 25 +++++++++++++++++++ 3 files changed, 41 insertions(+) diff --git a/sdk/include/opentelemetry/sdk/metrics/export/periodic_exporting_metric_reader.h b/sdk/include/opentelemetry/sdk/metrics/export/periodic_exporting_metric_reader.h index 93c183f83f..36f1b3f43b 100644 --- a/sdk/include/opentelemetry/sdk/metrics/export/periodic_exporting_metric_reader.h +++ b/sdk/include/opentelemetry/sdk/metrics/export/periodic_exporting_metric_reader.h @@ -36,6 +36,8 @@ class PeriodicExportingMetricReader : public MetricReader const PeriodicExportingMetricReaderOptions &options, const PeriodicExportingMetricReaderRuntimeOptions &runtime_options); + ~PeriodicExportingMetricReader() override; + AggregationTemporality GetAggregationTemporality( InstrumentType instrument_type) const noexcept override; diff --git a/sdk/src/metrics/export/periodic_exporting_metric_reader.cc b/sdk/src/metrics/export/periodic_exporting_metric_reader.cc index 215526505b..011c73737e 100644 --- a/sdk/src/metrics/export/periodic_exporting_metric_reader.cc +++ b/sdk/src/metrics/export/periodic_exporting_metric_reader.cc @@ -74,6 +74,20 @@ PeriodicExportingMetricReader::PeriodicExportingMetricReader( } } +PeriodicExportingMetricReader::~PeriodicExportingMetricReader() +{ + if (!IsShutdown()) + { + Shutdown(); + } + + if (worker_thread_.joinable()) + { + cv_.notify_all(); + worker_thread_.join(); + } +} + AggregationTemporality PeriodicExportingMetricReader::GetAggregationTemporality( InstrumentType instrument_type) const noexcept { diff --git a/sdk/test/metrics/periodic_exporting_metric_reader_test.cc b/sdk/test/metrics/periodic_exporting_metric_reader_test.cc index 2b817276e8..c2d36c69bf 100644 --- a/sdk/test/metrics/periodic_exporting_metric_reader_test.cc +++ b/sdk/test/metrics/periodic_exporting_metric_reader_test.cc @@ -117,6 +117,31 @@ TEST(PeriodicExportingMetricReader, Timeout) reader->Shutdown(); } +TEST(PeriodicExportingMetricReader, DestroyWithoutShutdown) +{ + // Verify that destroying a reader without calling Shutdown() does not cause + // use-after-destroy races on the condition variable / mutex used by the + // background worker thread. Before the destructor fix this test would fail + // under ThreadSanitizer with: + // WARNING: ThreadSanitizer: unlock of an unlocked mutex (or by a wrong thread) + std::unique_ptr exporter( + new MockPushMetricExporter(std::chrono::milliseconds{0})); + PeriodicExportingMetricReaderOptions options; + options.export_timeout_millis = std::chrono::milliseconds(200); + options.export_interval_millis = std::chrono::milliseconds(500); + // producer must be declared before reader so it outlives it — the reader's + // destructor joins the background thread which may still call Produce(). + MockMetricProducer producer; + { + auto reader = + std::make_shared(std::move(exporter), options); + reader->SetMetricProducer(&producer); + // Let the background thread start and enter its wait loop. + std::this_thread::sleep_for(std::chrono::milliseconds(50)); + // reader goes out of scope here — no Shutdown() call. + } +} + TEST(PeriodicExportingMetricReaderOptions, UsesEnvVars) { const char *env_interval = "OTEL_METRIC_EXPORT_INTERVAL"; From 13ec3036b94a47ca14ec9bd49b200178a534ff36 Mon Sep 17 00:00:00 2001 From: Jonathan Nillius Date: Fri, 17 Apr 2026 08:01:17 +0200 Subject: [PATCH 2/3] Address review feedback - Delete copy/move constructors and assignment operators to satisfy cppcoreguidelines-special-member-functions (Rule of Five). - Remove redundant worker_thread_.join() from destructor; Shutdown() already calls OnShutDown() which joins the thread. - Use std::make_unique in DestroyWithoutShutdown test. --- .../sdk/metrics/export/periodic_exporting_metric_reader.h | 5 +++++ sdk/src/metrics/export/periodic_exporting_metric_reader.cc | 6 ------ sdk/test/metrics/periodic_exporting_metric_reader_test.cc | 6 ++---- 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/export/periodic_exporting_metric_reader.h b/sdk/include/opentelemetry/sdk/metrics/export/periodic_exporting_metric_reader.h index 36f1b3f43b..b8dcc509b4 100644 --- a/sdk/include/opentelemetry/sdk/metrics/export/periodic_exporting_metric_reader.h +++ b/sdk/include/opentelemetry/sdk/metrics/export/periodic_exporting_metric_reader.h @@ -38,6 +38,11 @@ class PeriodicExportingMetricReader : public MetricReader ~PeriodicExportingMetricReader() override; + PeriodicExportingMetricReader(const PeriodicExportingMetricReader &) = delete; + PeriodicExportingMetricReader &operator=(const PeriodicExportingMetricReader &) = delete; + PeriodicExportingMetricReader(PeriodicExportingMetricReader &&) = delete; + PeriodicExportingMetricReader &operator=(PeriodicExportingMetricReader &&) = delete; + AggregationTemporality GetAggregationTemporality( InstrumentType instrument_type) const noexcept override; diff --git a/sdk/src/metrics/export/periodic_exporting_metric_reader.cc b/sdk/src/metrics/export/periodic_exporting_metric_reader.cc index 011c73737e..31d5d6484f 100644 --- a/sdk/src/metrics/export/periodic_exporting_metric_reader.cc +++ b/sdk/src/metrics/export/periodic_exporting_metric_reader.cc @@ -80,12 +80,6 @@ PeriodicExportingMetricReader::~PeriodicExportingMetricReader() { Shutdown(); } - - if (worker_thread_.joinable()) - { - cv_.notify_all(); - worker_thread_.join(); - } } AggregationTemporality PeriodicExportingMetricReader::GetAggregationTemporality( diff --git a/sdk/test/metrics/periodic_exporting_metric_reader_test.cc b/sdk/test/metrics/periodic_exporting_metric_reader_test.cc index c2d36c69bf..930f681622 100644 --- a/sdk/test/metrics/periodic_exporting_metric_reader_test.cc +++ b/sdk/test/metrics/periodic_exporting_metric_reader_test.cc @@ -124,8 +124,7 @@ TEST(PeriodicExportingMetricReader, DestroyWithoutShutdown) // background worker thread. Before the destructor fix this test would fail // under ThreadSanitizer with: // WARNING: ThreadSanitizer: unlock of an unlocked mutex (or by a wrong thread) - std::unique_ptr exporter( - new MockPushMetricExporter(std::chrono::milliseconds{0})); + auto exporter = std::make_unique(std::chrono::milliseconds{0}); PeriodicExportingMetricReaderOptions options; options.export_timeout_millis = std::chrono::milliseconds(200); options.export_interval_millis = std::chrono::milliseconds(500); @@ -133,8 +132,7 @@ TEST(PeriodicExportingMetricReader, DestroyWithoutShutdown) // destructor joins the background thread which may still call Produce(). MockMetricProducer producer; { - auto reader = - std::make_shared(std::move(exporter), options); + auto reader = std::make_shared(std::move(exporter), options); reader->SetMetricProducer(&producer); // Let the background thread start and enter its wait loop. std::this_thread::sleep_for(std::chrono::milliseconds(50)); From 00cfffb116d4a8a43d9ca62b5f5cc5f15e298003 Mon Sep 17 00:00:00 2001 From: Jonathan Nillius Date: Fri, 17 Apr 2026 08:19:47 +0200 Subject: [PATCH 3/3] Add CHANGELOG entry for PR #4008 --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 87a519613f..35cdcf2031 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,9 @@ Increment the: ## [Unreleased] +* [SDK] Fix PeriodicExportingMetricReader shutdown race on destruction + [#4008](https://github.com/open-telemetry/opentelemetry-cpp/pull/4008) + * [SDK] Move inline implementation from SDK headers to .cc files. Note: `GetEmptyAttributes()` now requires linking `opentelemetry_common`. [#3887](https://github.com/open-telemetry/opentelemetry-cpp/pull/3887)