diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f8f70a67d..a9a9c1da79 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) 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..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 @@ -36,6 +36,13 @@ class PeriodicExportingMetricReader : public MetricReader const PeriodicExportingMetricReaderOptions &options, const PeriodicExportingMetricReaderRuntimeOptions &runtime_options); + ~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 215526505b..31d5d6484f 100644 --- a/sdk/src/metrics/export/periodic_exporting_metric_reader.cc +++ b/sdk/src/metrics/export/periodic_exporting_metric_reader.cc @@ -74,6 +74,14 @@ PeriodicExportingMetricReader::PeriodicExportingMetricReader( } } +PeriodicExportingMetricReader::~PeriodicExportingMetricReader() +{ + if (!IsShutdown()) + { + Shutdown(); + } +} + 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..930f681622 100644 --- a/sdk/test/metrics/periodic_exporting_metric_reader_test.cc +++ b/sdk/test/metrics/periodic_exporting_metric_reader_test.cc @@ -117,6 +117,29 @@ 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) + 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); + // 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";