[BUG] MetricReader::Shutdown() logs "cannot invoke shutdown twice" but calls OnShutDown() again anyway

[brhenc]

Describe your environment

Reproduced by source read + build on main at 11fa0db0 (also present in v1.28.0, the latest release -- not a regression). sdk/src/metrics/metric_reader.cc, MetricReader::Shutdown.

Steps to reproduce

bool MetricReader::Shutdown(std::chrono::microseconds timeout) noexcept
{
  bool status = true;
  if (IsShutdown())
  {
    OTEL_INTERNAL_LOG_WARN("MetricReader::Shutdown - Cannot invoke shutdown twice!");
  }

  shutdown_.store(true, std::memory_order_release);

  if (!OnShutDown(timeout))
  {
    status = false;
    OTEL_INTERNAL_LOG_WARN("MetricReader::OnShutDown Shutdown failed. Will not be tried again!");
  }
  return status;
}

The if (IsShutdown()) block only logs a warning -- there's no return in it, so execution falls straight through to shutdown_.store(true, ...) and OnShutDown(timeout) regardless of whether this is the first call or the fifth. This isn't gated on a race; it happens on every single call after the first, deterministically.

PeriodicExportingMetricReader::OnShutDown() (the concrete implementation used by the periodic reader) joins its background worker thread. Calling Shutdown() twice therefore calls OnShutDown() -- and its join() -- twice. Joining an already-joined std::thread is undefined behavior (typically an std::system_error exception, itself another noexcept-boundary-crossing hazard given Shutdown is noexcept).

This is easy to hit in ordinary code, not just a deliberate double-call: it's common for application code to call Shutdown() explicitly during graceful termination and rely on the reader's destructor for defense-in-depth. Since #4008 added an explicit destructor to PeriodicExportingMetricReader that also calls Shutdown() (to fix a separate use-after-destroy issue), any code that already called Shutdown() once and then lets the reader go out of scope now hits this exact double-invocation path on totally normal teardown -- not just a misuse case.

What is the expected behavior?

A second Shutdown() call should be a safe no-op: log the warning (as it already does) and return without touching OnShutDown()/the worker thread again.

What is the actual behavior?

The warning is logged, but OnShutDown() (and whatever join/cleanup it performs) runs again anyway.

Additional context

Suggested fix -- replace the check-then-set with an atomic compare-and-swap so the "already shut down" branch actually short-circuits (shutdown_ is already std::atomic<bool>, so this is a pure reordering, not a new field):

 bool MetricReader::Shutdown(std::chrono::microseconds timeout) noexcept
 {
-  bool status = true;
-  if (IsShutdown())
+  bool status  = true;
+  bool expected = false;
+  if (!shutdown_.compare_exchange_strong(expected, true, std::memory_order_acq_rel))
   {
     OTEL_INTERNAL_LOG_WARN("MetricReader::Shutdown - Cannot invoke shutdown twice!");
+    return false;
   }
 
-  shutdown_.store(true, std::memory_order_release);
-
   if (!OnShutDown(timeout))
   {
     status = false;

Compile-checked against a clean build of this file -- no warnings or errors. Happy to open a PR with this if useful.

[mateenali66]

The missing return is there, but the double-join consequence does not reach either in-tree reader. PeriodicExportingMetricReader::OnShutDown guards with if (worker_thread_.joinable()), which is false after the first join, and PrometheusExporter::OnShutDown null-checks exposer_. What does happen on a second call is exporter_->Shutdown(timeout) running again, and whether that is safe is per-exporter rather than something the SDK guarantees.

So it reads as a base-class contract defect rather than an in-tree crash. An out-of-tree MetricReader whose OnShutDown joins unguarded would hit exactly what you describe.

Is the intended fix an early return in MetricReader::Shutdown, or documenting that OnShutDown has to be idempotent?

Suggest triage/accepted.