[Profiler] Fix start/stop crash at shutdown - #9037
Open
gleocadie wants to merge 7 commits into
Open
Conversation
github-actions
Bot
added
the
area:profiler
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9037) and master. ✅ No regressions detected |
BenchmarksBenchmark execution time: 2026-08-13 17:24:21 Comparing candidate commit 28b84df in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 71 metrics, 0 unstable metrics, 65 known flaky benchmarks, 61 flaky benchmarks without significant changes.
|
gleocadie
marked this pull request as ready for review
August 13, 2026 10:59
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38af141644
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
StartServices() can never run more than once per process: it's called either from Initialize() (ManuallyEnabled, itself a one-time CLR callback) or from OnStartDelayedProfiling(), which SsiManager's StartProfiling() wraps in std::call_once. A service that fails Start() is not retried within the same process - the profiler is simply not started, and a subsequent attempt only happens in a fresh process. So the "clean up a leftover thread from a previous timed-out attempt before reassigning _pWatcherThread" guard added for the earlier codex comment was defending against a retry that cannot happen here. Removed, along with the comments justifying it on that now-known-incorrect premise. The other half of that fix stays: requesting shutdown on timeout so the thread doesn't run unsupervised, since that's needed regardless of retries - StopServices()'s error-cleanup path (triggered by some *other* service failing in the same StartServices() pass) hits the same Stop()-CAS-guard-skips-StopImpl() gap without any retry involved. Verified: full native test suite passes (579/580, 1 pre-existing unrelated skip).
chrisnas
approved these changes
Aug 13, 2026
chrisnas
left a comment
chrisnas
left a comment
Contributor
There was a problem hiding this comment.
LGTM except a few over-commented code
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Fixes a shutdown-time SIGSEGV in the Continuous Profiler:
StackSamplerLoopis promoted from a private implementation detail ofStackSamplerLoopManagerto its own independently-registeredIService, so its start/stop lifecycle is owned directly byCorProfilerCallback's existingStartServices()/StopServices()machinery instead of an ad hoc, asynchronous internal sequence. Two related data races (volatile bool→std::atomic<bool>) are also fixed, found while adding tests for the new lifecycle.Reason for change
A customer crash report showed the sampler thread segfaulting inside
StackSamplerLoop::CodeHotspotIteration()while the process's main thread was, at the same instant, blocked in CLR shutdown joining that same thread. Two compounding issues caused it:StackSamplerLoop::Start()was called asynchronously from inside a freshly-spawned watcher thread, with nothing guaranteeing it had actually run beforeStackSamplerLoopManager::StartImpl()returned.CorProfilerCallback::Shutdown()callsStackSamplerLoopManager::Stop()explicitly and early. If the watcher thread hadn't reachedStart()yet (an unbounded race, worse under CPU contention), thatStop()silently no-opped — the sampler's one-shot guard rejected it — and the watcher went on to start it anyway, after shutdown had already begun tearing down theManagedThreadListinstances it depends on. The only thing left to stop it at that point was~StackSamplerLoop()'s last-resort destructor call, by which time those dependencies could already be gone.Implementation details
StackSamplerLoopbecomes a real service. It already inheritedServiceBase; it just wasn't registered.CorProfilerCallback::InitializeServices()now registers it right afterStackSamplerLoopManager:ManagedThreadList'sStop()/destruction, since that was registered much earlier and so is stopped much later — guaranteeing thesampler is fully dead before anything it depends on is touched.
RunWatcher()(renamed fromRunWatcherAndSampler()) spawns the watcher thread and blocks on astd::promise/std::futurehandshake until it signals it's runn idiomManagedCodeCache::Initialize()already uses elsewhere. Atimeout logs an error and returnsfalseinstead of hangingInitialize()forever; the possibly-still-starting thread is joined later, deferred toStop()orthe destructor.
StackSamplerLooptakes aStackSamplerLoopManager*at construction; the manager gets aStackSamplerLoop*back via a one-timeSetStackSamplerLoop()setter, since the loop doesn't exist yet tartImpl()` checks this was actually called and fails cleanly witha logged error rather than null-deref'ing if a future refactor drops it.StackFramesCollectorBase/CallstackProrCallback(constructed once, shared by both services as rawnon-owning pointers), since the manager no longer constructsStackSamplerLoopitself.StackSamplerLoop::_shutdownRequestedandStackSamplerLoopManager::_isWatcherShutdownRequested, bothvolatile bool→std::atomic<bool>. Straight typeswap, no behavior change.
CorProfilerCallback::Shutdown()'s existing earlyStop()call (needed so the final.pprofcaptures the last samples) now stops both services explicitly;StopServices()calling them again later is a guaranteed no-op.Test coverage
New file
StackSamplerLoopManagerTest.cppexercises the new lifeng a fullCorProfilerCallback:StartFailsCleanlyWhenStackSamplerLoopWasNeverWiredIn— the safety-net check whenSetStackSamplerLoop()is skipped.StackSamplerLoopStartsAndStopsIndependentlyOfTheManager— thet/stop lifecycle, testable in isolation from the manager.FullLifecycleMatchesStartServicesAndStopServicesOrdering— full start (forward order) / stop (reverse order) cycle matching production'sStartServices()/StopServices()sequencing.Worth running locally with TSan (
-DRUN_TSAN=1) to confirm the tob currently filters to*RingBuffer*only, so it won't exercisethese new tests until that filter is widened.Other details