Fix EventPipe dispose-in-callback deadlock

[mdh1418]

Fixes #106087

Summary

This PR prevents deadlocks when an EventSource is disposed from an EventSource callback, without broadly prohibiting all callback-time disposal.

The original failure occurs when disposal reaches native provider unregistration while a tracing callback is active. Native unregistration can wait for callbacks to complete, but the current callback cannot complete until Dispose() returns.

The resulting behavior is:

• Disposing an EventSource that has an active callback from within an EventSource callback throws InvalidOperationException instead of deadlocking.
• A callback for source A may still dispose inactive source B.
• Disposal from an ordinary thread closes callback admission and waits for callbacks already admitted for that source to finish.
• Once disposal has claimed a source, no additional callbacks are admitted.

Implementation

Per-EventSource callback gate

Each EventSource has an atomic callback state:

• 0..int.MaxValue: the number of admitted callbacks.
• int.MinValue + n: disposal has claimed the source, callback admission is closed, and n admitted callbacks remain.
• int.MinValue: disposal has claimed the source and all admitted callbacks have drained.

Dispose() atomically claims the source before teardown. If an ordinary thread observes active callbacks, it waits for them to drain. If a thread already executing an EventSource callback attempts to dispose a source with active callbacks, it throws rather than performing a wait that could deadlock.

A thread-static callback depth tracks whether the current thread is anywhere within a possibly nested EventSource callback chain. The per-source state identifies whether the target source is active; the thread-static depth identifies whether the disposing thread may participate in the callback dependency.

Managed callback lifetime

The managed drain is needed in addition to native provider unregistration. Native unregistration covers native ETW/EventPipe callbacks, but it does not cover every managed callback path. Without the managed drain, callbacks already admitted, or blocked waiting for EventListenersLock, could execute while Dispose() disables and clears providers or after disposal returns.

The gate is applied to:

• Normal command dispatch.
• Deferred command replay during source initialization.
• Provider publication and OnEventSourceCreated.
• Existing-source listener catch-up.

Native unregister deferral

Native callbacks may already have been queued before reaching the managed callback gate. Therefore, when provider disposal occurs from an EventSource callback, native unregister is deferred to the thread pool.

This logic lives in the shared EventProvider implementation, so it applies to both EventPipe and ETW. Its state machine ensures only one thread performs unregister, permits finalization to retry after a failed deferred attempt, and prevents exceptions from escaping thread-pool or finalizer paths.

Tests

Added coverage for:

• Same-source disposal from OnEventCommand.
• Disposal of inactive source B from source A's callback.
• Ordinary concurrent disposal waiting for an active callback.
• Deferred command callbacks.
• Provider publication callbacks.
• Listener catch-up callbacks.
• Nested callback chains.

Validation completed:

• Checked System.Private.CoreLib build.
• Full System.Diagnostics.Tracing.Tests: 61 total, 0 failed, 7 skipped.
• Standalone EventPipe scenario where A's callback disposes inactive B: completed.
• Standalone nested scenario where A's callback enters B's callback and B disposes active A: rejected without deadlock.

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

Tagging subscribers to this area: @steveisok, @dotnet/area-system-diagnostics-tracing
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

Fixes a potential self-deadlock when EventPipeEventProvider.Unregister() is invoked re-entrantly from within the provider callback (e.g., EventSource.Dispose() called from OnEventCommand). The change tracks the currently executing provider callback per-thread and defers provider deletion to a background work item to ensure the callback can return before ep_delete_provider waits for in-flight callbacks.

Changes:

• Track the currently executing provider callback via a [ThreadStatic] field and use it to detect re-entrant Unregister() calls.
• Defer EventPipeInternal.DeleteProvider (and GCHandle disposal) to a ThreadPool work item when Unregister() is called from within the callback for that provider.
• Add a regression test that starts an EventPipe session, triggers OnEventCommand, and verifies Dispose() completes without hanging.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventPipeEventProvider.cs	Adds re-entrancy detection for callbacks and defers provider deletion to avoid self-deadlock.
src/libraries/System.Diagnostics.Tracing/tests/System.Diagnostics.Tracing.Tests.csproj	Includes the new callback/dispose regression test source file in the test project.
src/libraries/System.Diagnostics.Tracing/tests/BasicEventSourceTest/TestCallbacks.cs	Adds an xUnit regression test that reproduces the dispose-in-OnEventCommand deadlock scenario via EventPipe.

Comments suppressed due to low confidence (2)

src/libraries/System.Diagnostics.Tracing/tests/BasicEventSourceTest/TestCallbacks.cs:121

• This regression test is validating a deadlock scenario, but if the deadlock regresses, the test can still hang the test process: session.Stop() has no timeout and is executed on the test thread. Consider running this repro under RemoteExecutor with a bounded RemoteInvokeOptions.TimeOut, or otherwise ensuring Stop/cleanup is time-bounded so the test fails rather than hanging CI when the bug is present.

            // Wait for Dispose() to complete; if it deadlocks, this will timeout.
            bool disposed = disposeCompleted.Wait(TimeSpan.FromSeconds(30));

            session.Stop();
            readerTask.Wait(TimeSpan.FromSeconds(5));

src/libraries/System.Diagnostics.Tracing/tests/BasicEventSourceTest/TestCallbacks.cs:122

• readerTask.Wait(TimeSpan.FromSeconds(5)) ignores the return value. If the reader never completes, the test will still pass (as long as Dispose completes) and can leave a stuck background task that may interfere with subsequent tests. Consider asserting the wait result so failures are surfaced deterministically.

            session.Stop();
            readerTask.Wait(TimeSpan.FromSeconds(5));

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/libraries/System.Diagnostics.Tracing/tests/BasicEventSourceTest/TestCallbacks.cs:123

• This test can still hang the test process if the regression reappears: StartEventPipeSession enables providers synchronously, so the Dispose-in-OnEventCommand deadlock can block before you ever reach the disposeCompleted.Wait(...) timeout. Consider starting the session on a background task and applying a timeout to the session-start step, so the test fails deterministically instead of hanging indefinitely.

            var providers = new[] { new EventPipeProvider(source.Name, System.Diagnostics.Tracing.EventLevel.Verbose, long.MaxValue) };
            var client = new DiagnosticsClient(Environment.ProcessId);
            using var session = client.StartEventPipeSession(providers, requestRundown: false);

            // Drain the event stream in a background thread so the runtime's buffer doesn't fill up

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs:1536

• Dispose(bool) now throws InvalidOperationException whenever the current thread holds EventListener.EventListenersLock, even when disposing==false. The method’s own remarks explicitly say to avoid throwing when called from the finalizer (disposing=false), and this check is only relevant to the managed-dispose path where the providers are disposed.

            // Do not invoke Dispose under the lock as this can lead to a deadlock.
            // See https://github.com/dotnet/runtime/issues/48342 for details.
            if (Monitor.IsEntered(EventListener.EventListenersLock))
            {
                throw new InvalidOperationException(SR.EventSource_DisposeInsideCallback);
            }

src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs:1506

• The implementation now rejects Dispose() when called from within callbacks (by throwing), but the PR description states Dispose/Unregister are deferred so disposal can complete without blocking. Please align the PR description (and/or the implementation) with the actual behavior change being introduced here.

        /// <exception cref="InvalidOperationException">
        /// The method was called while an <see cref="EventSource"/> callback was executing.
        /// </exception>

src/libraries/System.Diagnostics.Tracing/tests/System.Diagnostics.Tracing.Tests.csproj:41

• The entries in this ItemGroup are kept in filename order; the new TestCallbacks.cs entry should be placed before TestEventCounter.cs/TestFilter.cs to preserve the existing ordering convention (keeps diffs stable and reduces merge conflicts).

    <Compile Include="BasicEventSourceTest\Harness\Listeners.cs" />
    <Compile Include="BasicEventSourceTest\TestEventCounter.cs" />
    <Compile Include="BasicEventSourceTest\TestFilter.cs" />
    <Compile Include="BasicEventSourceTest\TestCallbacks.cs" />
    <Compile Include="BasicEventSourceTest\TestNotSupported.cs" />

src/libraries/System.Diagnostics.Tracing/tests/BasicEventSourceTest/TestCallbacks.cs:120

• This test currently ignores the return values from callbackCompleted.Wait(...) and readerTask.Wait(...), and it calls session.Stop() unconditionally. If the regression reappears, session.Stop()/Dispose may block and hang the test run. Prefer asserting the waits and bounding Stop() with a timeout so failures fail fast instead of hanging CI.

            bool completed = callbackCompleted.Wait(TimeSpan.FromSeconds(30));

            session.Stop();
            readerTask.Wait(TimeSpan.FromSeconds(5));

[noahfalk]

I don't think there's a deadlock for trying to dispose EventSource B inside a callback for EventSource A since there wouldn't be any callbacks inflight for EventSource B, and so EventSource B can dispose immediately.

Have you tested it on EventPipe and ETW? I'm happy to be wrong, I just didn't want to under-scope the fix.

[noahfalk]

We use this lock for more than just callbacks. I think we should use a thread-local variable to track when we are inside a callback. You could check out what EventSource function the ETW and EventPipeProvider calls to dispatch a callback and observe it there.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventProvider.cs:158

• If Dispose(true) runs in a callback (deferred unregister) or if _eventProvider.Unregister() throws, _disposed is already set to true. Subsequent Dispose(true) calls then return immediately without attempting to finish unregister, which can leave the provider registered and the finalizer unsuppressed indefinitely unless the queued work happens to succeed. The early-return path should still try to complete (or re-queue) unregistration when it hasn’t completed yet.

            if (_disposed)
            {
                if (!disposing)
                {
                    _ = TryCompleteUnregister(throwOnFailure: false);
                }

                return;
            }

src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs:1587

• The PR description and linked issue describe making Dispose() complete (avoid hang) when called from inside OnEventCommand, but this change instead throws InvalidOperationException when Dispose is invoked from within an EventSource callback for the same source (via IsExecutingCallback + callback admission). Please reconcile intent: either update the PR narrative/issue expectations to reflect the new exception behavior, or adjust the implementation to make Dispose non-blocking in this scenario.

                if (IsExecutingCallback)
                {
                    // Waiting here would deadlock if this thread is responsible for completing a callback.
                    throw new InvalidOperationException(SR.EventSource_DisposeInsideCallback);
                }

[noahfalk]

Suggested change

	int callbackState = Volatile.Read(ref m_eventSourceCallbackState);
	if (IsExecutingCallback)
	{
	// Waiting here would deadlock if this thread is responsible for completing a callback.
	throw new InvalidOperationException(SR.EventSource_DisposeInsideCallback);
	}

Other than throwing this exception its not clear to me what benefit we get from the other changes in the PR. I would expect the underlying EventPipe/ETW unregister call already blocks until callbacks are complete.

I think you could call Enter/ExitCallbackScope() inside EventSource.SendCommand() and nowhere else. If folks want to call EventSource.Dispose() from defered commands run during constructor initialize or in EventListener callbacks like EventSourceCreated its weird and there might even be other bugs in how we handle it, but I don't think it deadlocks which was the scope of the current bug.

Hopefully that substantially simplifies this change.

[noahfalk]

I don't think we should do this deferal. Unregister is intended to be a deterministic cleanup that ensures no callbacks are running for provider after it returns so doing it lazily would undercut that goal.

Also I'd say its the callers job not to call while the current thread is inside a callback from the provider. Its not clear how execution would get here with the check we added to EventSource.Dispose() but if it does I'd just let it deadlock as before.