[Profiler] Fix start/stop crash at shutdown

profiler/src/ProfilerEngine/Datadog.Profiler.Native/CorProfilerCallback.cpp

@@ -52,6 +52,8 @@
 #include "Sample.h"
 #include "SampleValueTypeProvider.h"
 #include "SsiManager.h"
+#include "StackFramesCollectorBase.h"
+#include "StackSamplerLoop.h"
 #include "StackSamplerLoopManager.h"
 #include "ThreadsCpuManager.h"
 #include "WallTimeProvider.h"
@@ -613,18 +615,37 @@ void CorProfilerCallback::InitializeServices()
     auto const& sampleTypeDefinitions = valueTypeProvider.GetValueTypes();
     Sample::ValuesCount = sampleTypeDefinitions.size();
 
+    // Stack frames collector is shared by StackSamplerLoopManager and StackSamplerLoop
+    //  (registered separately below), so it's constructed here rather than by either of them.
+    _callstackProvider = CallstackProvider(_memoryResourceManager.GetSynchronizedPool(100, Callstack::MaxSize));
+    _pStackFramesCollector = OsSpecificApi::CreateNewStackFramesCollectorInstance(
+        _pCorProfilerInfo, _pConfiguration.get(), &_callstackProvider, _metricsRegistry);
+
     _pStackSamplerLoopManager = RegisterService<StackSamplerLoopManager>(
         _pCorProfilerInfo,
-        _pConfiguration.get(),
         _metricsSender,
         _pClrLifetime.get(),
         _pThreadsCpuManager,
+        _pStackFramesCollector.get(),
+        _metricsRegistry);
+
+    _pStackSamplerLoop = RegisterService<StackSamplerLoop>(
+        _pCorProfilerInfo,
+        _pConfiguration.get(),
+        _pStackFramesCollector.get(),
+        _pStackSamplerLoopManager,
+        _pThreadsCpuManager,
         _pManagedThreadList,
         _pCodeHotspotsThreadList,
         _pWallTimeProvider,
         _pCpuTimeProvider,
-        _metricsRegistry,
-        CallstackProvider(_memoryResourceManager.GetSynchronizedPool(100, Callstack::MaxSize)));
+        _metricsRegistry);
+
+    // Completes the wiring: StackSamplerLoop needed StackSamplerLoopManager to already exist (just
+    // passed above), but the manager needs StackSamplerLoop back (for its Windows deadlock-progress
+    // check) and that couldn't be constructor-injected the other way around, since StackSamplerLoop
+    // didn't exist yet when the manager was constructed.
+    _pStackSamplerLoopManager->SetStackSamplerLoop(_pStackSamplerLoop);
 
 #ifdef ARM64
     if (Log::IsDebugEnabled())
@@ -972,6 +993,7 @@ bool CorProfilerCallback::DisposeServices()
 
     _pThreadsCpuManager = nullptr;
     _pStackSamplerLoopManager = nullptr;
+    _pStackSamplerLoop = nullptr;
     _pManagedThreadList = nullptr;
     _pNativeThreadList = nullptr;
     _pCodeHotspotsThreadList = nullptr;
@@ -1819,6 +1841,9 @@ HRESULT STDMETHODCALLTYPE CorProfilerCallback::Shutdown()
 
     // A final .pprof should be generated before exiting
     // The aggregator must be stopped before the provider, since it will call them to get the last samples
+    // (StopServices() will also call Stop() on both of these again later, in the correct reverse-of-
+    // registration order; both are one-shot services, so those later calls are guaranteed no-ops.)
+    _pStackSamplerLoop->Stop();
     _pStackSamplerLoopManager->Stop();
 
 

profiler/src/ProfilerEngine/Datadog.Profiler.Native/CorProfilerCallback.h

@@ -10,6 +10,7 @@
 
 #include "AllocationsProvider.h"
 #include "ApplicationStore.h"
+#include "CallstackProvider.h"
 #include "EventPipeEventsManager.h"
 #include "ExceptionsProvider.h"
 #include "IAppDomainStore.h"
@@ -57,7 +58,9 @@ class IService;
 class IThreadsCpuManager;
 class IManagedThreadList;
 class INativeThreadList;
+class StackSamplerLoop;
 class StackSamplerLoopManager;
+class StackFramesCollectorBase;
 class IConfiguration;
 class IExporter;
 class RawSampleTransformer;
@@ -250,6 +253,7 @@ private :
     // Their lifetime is managed by the _services vector.
     IThreadsCpuManager* _pThreadsCpuManager = nullptr;
     StackSamplerLoopManager* _pStackSamplerLoopManager = nullptr;
+    StackSamplerLoop* _pStackSamplerLoop = nullptr;
     IManagedThreadList* _pManagedThreadList = nullptr;
     IManagedThreadList* _pCodeHotspotsThreadList = nullptr;
     IApplicationStore* _pApplicationStore = nullptr;
@@ -279,6 +283,13 @@ private :
 
     std::vector<std::unique_ptr<IService>> _services;
 
+    // Shared by StackSamplerLoopManager and StackSamplerLoop (both registered in _services above).
+    // StackFramesCollectorBase is not itself an IService (no Start/Stop lifecycle), so it - and the
+    // CallstackProvider backing it - are owned directly here instead, and handed to both as a raw,
+    // non-owning pointer.
+    CallstackProvider _callstackProvider;
+    std::unique_ptr<StackFramesCollectorBase> _pStackFramesCollector;
+
     std::unique_ptr<IExporter> _pExporter = nullptr;
     std::shared_ptr<IConfiguration> _pConfiguration = nullptr;
     bool _IsManagedConfigurationSet = false; // profiler can't start before this becomes true

profiler/src/ProfilerEngine/Datadog.Profiler.Native/StackSamplerLoop.h

@@ -8,6 +8,7 @@
 #include <atlcomcli.h>
 #endif
 
+#include <atomic>
 #include <memory>
 #include <unordered_map>
 
@@ -77,7 +78,7 @@ class StackSamplerLoop : public ServiceBase
 
     std::unique_ptr<std::thread> _pLoopThread;
     DWORD _loopThreadOsId;
-    volatile bool _shutdownRequested = false;
+    std::atomic<bool> _shutdownRequested = false;
     std::shared_ptr<ManagedThreadInfo> _targetThread;
     uint32_t _iteratorWallTime;
     uint32_t _iteratorCpuTime;

profiler/src/ProfilerEngine/Datadog.Profiler.Native/StackSamplerLoopManager.cpp

@@ -14,6 +14,9 @@ constexpr std::chrono::milliseconds DeadlockDetectionInterval = 1s;
 constexpr std::chrono::milliseconds MaxExpectedStackSampleCollectionDurationMs = 500ms;
 constexpr std::chrono::nanoseconds CollectionDurationThresholdNs = std::chrono::nanoseconds(MaxExpectedStackSampleCollectionDurationMs);
 
+// Make sure the Watcher thread does not hang Start() forever.
+constexpr std::chrono::seconds WatcherStartupTimeout = 2s;
+
 #ifdef NDEBUG
 constexpr std::chrono::milliseconds StatsAggregationPeriodMs = 30000ms;
 #else
@@ -26,25 +29,15 @@ const std::chrono::nanoseconds StackSamplerLoopManager::StatisticAggregationPeri
 
 StackSamplerLoopManager::StackSamplerLoopManager(
     ICorProfilerInfo4* pCorProfilerInfo,
-    IConfiguration* pConfiguration,
     std::shared_ptr<IMetricsSender> metricsSender,
     IClrLifetime const* clrLifetime,
     IThreadsCpuManager* pThreadsCpuManager,
-    IManagedThreadList* pManagedThreadList,
-    IManagedThreadList* pCodeHotspotThreadList,
-    ICollector<RawWallTimeSample>* pWallTimeCollector,
-    ICollector<RawCpuSample>* pCpuTimeCollector,
-    MetricsRegistry& metricsRegistry,
-    CallstackProvider callstackProvider
+    StackFramesCollectorBase* pStackFramesCollector,
+    MetricsRegistry& metricsRegistry
     ) :
     _pCorProfilerInfo{pCorProfilerInfo},
-    _pConfiguration{pConfiguration},
     _pThreadsCpuManager{pThreadsCpuManager},
-    _pManagedThreadList{pManagedThreadList},
-    _pCodeHotspotsThreadList{pCodeHotspotThreadList},
-    _pWallTimeCollector{pWallTimeCollector},
-    _pCpuTimeCollector{pCpuTimeCollector},
-    _pStackFramesCollector{nullptr},
+    _pStackFramesCollector{pStackFramesCollector},
     _pStackSamplerLoop{nullptr},
     _deadlockInterventionInProgress{0},
     _pWatcherThread{nullptr},
@@ -59,12 +52,9 @@ StackSamplerLoopManager::StackSamplerLoopManager(
     _totalDeadlockDetectionsCount{0},
     _metricsSender{metricsSender},
     _statisticsReadyToSend{nullptr},
-    _metricsRegistry{metricsRegistry},
-    _callstackProvider{std::move(callstackProvider)}
+    _metricsRegistry{metricsRegistry}
 {
     _pCorProfilerInfo->AddRef();
-    _pStackFramesCollector = OsSpecificApi::CreateNewStackFramesCollectorInstance(
-        _pCorProfilerInfo, pConfiguration, &_callstackProvider, _metricsRegistry);
 
     _currentStatistics = std::make_unique<Statistics>();
     _statisticCollectionStartNs = OpSysTools::GetHighPrecisionNanoseconds();
@@ -77,6 +67,14 @@ StackSamplerLoopManager::~StackSamplerLoopManager()
     // Just in case it was not called explicitely
     Stop();
 
+    // If Start() timed out waiting for the watcher thread (see RunWatcher()) but the thread was
+    // nonetheless successfully created, Stop()'s one-shot guard above is a no-op (this service's
+    // state never reached Started), so ShutdownWatcher() would otherwise never run - leaving
+    // _pWatcherThread joinable here, which would call std::terminate() when it is destroyed right
+    // after this destructor returns. Calling it unconditionally is a safe no-op if Stop() already
+    // handled it (ShutdownWatcher() resets _pWatcherThread to null after a successful join).
+    ShutdownWatcher();
+
     ICorProfilerInfo4* pCorProfilerInfo = _pCorProfilerInfo;
     if (pCorProfilerInfo != nullptr)
     {
@@ -90,45 +88,59 @@ const char* StackSamplerLoopManager::GetName()
     return _serviceName;
 }
 
+void StackSamplerLoopManager::SetStackSamplerLoop(StackSamplerLoop* pStackSamplerLoop)
+{
+    _pStackSamplerLoop = pStackSamplerLoop;
+}
+
 bool StackSamplerLoopManager::StartImpl()
 {
-    InitializeSampler();
-    RunWatcherAndSampler();
+    if (_pStackSamplerLoop == nullptr)
+    {
+        Log::Error("StackSamplerLoopManager::StartImpl - StackSamplerLoop was not wired in "
+                   "(SetStackSamplerLoop() must be called before Start()). This is a bug.");
+        return false;
+    }
 
-    return true;
+    return RunWatcher();
 }
 
 bool StackSamplerLoopManager::StopImpl()
 {
-    _pStackSamplerLoop->Stop();
-
     ShutdownWatcher();
 
     return true;
 }
 
-void StackSamplerLoopManager::InitializeSampler()
+bool StackSamplerLoopManager::RunWatcher()
 {
-    _pStackSamplerLoop = std::make_unique<StackSamplerLoop>(
-        _pCorProfilerInfo,
-        _pConfiguration,
-        _pStackFramesCollector.get(),
-        this,
-        _pThreadsCpuManager,
-        _pManagedThreadList,
-        _pCodeHotspotsThreadList,
-        _pWallTimeCollector,
-        _pCpuTimeCollector,
-        _metricsRegistry);
-}
+    std::promise<void> watcherReadyPromise;
+    std::future<void> watcherReadyFuture = watcherReadyPromise.get_future();
 
-void StackSamplerLoopManager::RunWatcherAndSampler()
-{
-    _pWatcherThread = std::make_unique<std::thread>([this]
-        {
-            OpSysTools::SetNativeThreadName(WatcherThreadName);
-            WatcherLoop();
-        });
+    try
+    {
+        _pWatcherThread = std::make_unique<std::thread>(
+            [this, promise = std::move(watcherReadyPromise)]() mutable
+            {
+                OpSysTools::SetNativeThreadName(WatcherThreadName);
+                WatcherLoop(std::move(promise));
+            });
+    }
+    catch (const std::exception& ex)
+    {
+        Log::Error("StackSamplerLoopManager::RunWatcher - Failed to create the watcher thread: ", ex.what());
+        return false;
+    }
+
+    // Wait for the watcher thread to be ready before returning
+    if (watcherReadyFuture.wait_for(WatcherStartupTimeout) == std::future_status::ready)
+    {
+        return true;
+    }
+
+    Log::Error("StackSamplerLoopManager::RunWatcher - Timed out after ", WatcherStartupTimeout.count(),
+               " seconds waiting for the watcher thread to start.");
+    return false;
 }
 
 void StackSamplerLoopManager::ShutdownWatcher()
@@ -137,17 +149,23 @@ void StackSamplerLoopManager::ShutdownWatcher()
     {
         _isWatcherShutdownRequested = true;
 
-        _pWatcherThread->join();
+        try
+        {
+            _pWatcherThread->join();
+            _pWatcherThread.reset();
+        }
+        catch (const std::exception&)
+        {
+        }
     }
 }
 
-void StackSamplerLoopManager::WatcherLoop()
+void StackSamplerLoopManager::WatcherLoop(std::promise<void> watcherReadyPromise)
 {
     Log::Info("StackSamplerLoopManager::WatcherLoop started.");
     _pThreadsCpuManager->Map(OpSysTools::GetThreadId(), WatcherThreadName);
 
-    // Start the sampler loop only when the watcher is ready
-    _pStackSamplerLoop->Start();
+    watcherReadyPromise.set_value();
 
     while (false == _isWatcherShutdownRequested)
     {