[SDK] Fix MetricReader post shutdown handling

[denizariyan]

Fixes #4536

Changes

• Run on shutdown method only once
• Also fix a similar "warn but don't actually do what you say you do" issue on force flush in metric reader

For significant contributions please make sure you have completed the following items:

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.47%. Comparing base (e0e0458) to head (7e75347).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4548      +/-   ##
==========================================
+ Coverage   86.45%   86.47%   +0.02%     
==========================================
  Files         525      525              
  Lines       20450    20453       +3     
==========================================
+ Hits        17679    17685       +6     
+ Misses       2771     2768       -3     

Files with missing lines	Coverage Δ
.../include/opentelemetry/sdk/metrics/metric_reader.h	100.00% <ø> (ø)
sdk/src/metrics/metric_reader.cc	91.38% <100.00%> (+5.93%)	⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mateenali66]

the CAS fixes the sequential double invoke, but a concurrent second caller now gets true back before the first has finished OnShutDown().

built this branch's metric_reader.cc against a reader whose OnShutDown() sleeps 300ms, thread B running 50ms behind thread A:

                            main    this PR   +mutex
second Shutdown() blocked   304ms   0ms       249ms
OnShutDown done by then     yes     NO        yes
OnShutDown invocations      2       1         1

BatchSpanProcessor::InternalShutdown avoids this by taking shutdown_m before the exchange (batch_span_processor.cc:383-384), so the second caller waits and then sees already_shutdown. the third column is that idiom in MetricReader.

ConcurrentShutdownIsInvokedOnce cannot catch it, since OnShutDown returns immediately there. it matters because ~PeriodicExportingMetricReader() skips Shutdown() when IsShutdown() is true (periodic_exporting_metric_reader.cc:77), so a caller acting on the early true can tear down mid join().

mutex, or document Shutdown() as not concurrency safe and keep the CAS?

[denizariyan]

the CAS fixes the sequential double invoke, but a concurrent second caller now gets true back before the first has finished OnShutDown().

built this branch's metric_reader.cc against a reader whose OnShutDown() sleeps 300ms, thread B running 50ms behind thread A:

                            main    this PR   +mutex
second Shutdown() blocked   304ms   0ms       249ms
OnShutDown done by then     yes     NO        yes
OnShutDown invocations      2       1         1

BatchSpanProcessor::InternalShutdown avoids this by taking shutdown_m before the exchange (batch_span_processor.cc:383-384), so the second caller waits and then sees already_shutdown. the third column is that idiom in MetricReader.

ConcurrentShutdownIsInvokedOnce cannot catch it, since OnShutDown returns immediately there. it matters because ~PeriodicExportingMetricReader() skips Shutdown() when IsShutdown() is true (periodic_exporting_metric_reader.cc:77), so a caller acting on the early true can tear down mid join().

mutex, or document Shutdown() as not concurrency safe and keep the CAS?

Thanks, I agree I think serializing the concurrent calls here is the less surprising implementation. It also matches what BSP does.

I changed it the impl to serialize each call to shutdown. This might be something we need to revisit when implementing timeout handling but I think is the better choice for now