fix(profiling): release the EventPipe session when the SDK shuts down

AGENTS.md

@@ -153,6 +153,17 @@ gh pr view --json number -q '.number'
 - Maintain **backwards compatibility** — avoid breaking public API without strong justification
 - Platform-specific code lives in `src/Sentry/Platforms/` and is conditionally compiled
 
+## Code Style: prefer no comments
+
+This repository favours clean, readable code that needs no comments at all. Reach for a
+better name or a smaller method before reaching for a comment. Where something genuinely
+isn't obvious — a non-intuitive framework behaviour, a workaround for an upstream bug — a
+minimal comment is fine, but the code and the PR description are the documentation.
+
+Do not add comments that restate what the code already says. In particular, don't annotate
+members as being exposed for tests (`// Exposed for tests`) — that's already apparent from
+the member being `internal` and from tests being its only callers.
+
 ## Adding New Options (AOT Compatibility)
 
 `SentryOptions` is **not** bound directly from configuration. Instead, a parallel `BindableSentryOptions` class (`src/Sentry/BindableSentryOptions.cs`) exists for AOT-safe configuration binding.

src/Sentry.Profiling/ProfilingIntegration.cs

@@ -6,10 +6,15 @@ namespace Sentry.Profiling;
 /// <summary>
 /// Enables transaction performance profiling.
 /// </summary>
-public class ProfilingIntegration : ISdkIntegration
+public class ProfilingIntegration : ISdkIntegration, IDisposable
 {
     private TimeSpan _startupTimeout;
 
+    private readonly object _lock = new();
+    private SentryOptions? _options;
+    private SamplingTransactionProfilerFactory? _ownedFactory;
+    private int _registrations;
+
     /// <summary>
     /// Initializes the profiling integration.
     /// </summary>
@@ -35,7 +40,21 @@ public void Register(IHub hub, SentryOptions options)
         {
             try
             {
-                options.TransactionProfilerFactory ??= new SamplingTransactionProfilerFactory(options, _startupTimeout);
+                lock (_lock)
+                {
+                    if (options.TransactionProfilerFactory is null)
+                    {
+                        var factory = new SamplingTransactionProfilerFactory(options, _startupTimeout);
+                        options.TransactionProfilerFactory = factory;
+                        _options = options;
+                        _ownedFactory = factory;
+                        _registrations = 1;
+                    }
+                    else if (ReferenceEquals(options.TransactionProfilerFactory, _ownedFactory))
+                    {
+                        _registrations++;
+                    }
+                }
             }
             catch (Exception e)
             {
@@ -47,4 +66,30 @@ public void Register(IHub hub, SentryOptions options)
             options.LogInfo("Profiling Integration is disabled because profiling is disabled by configuration.");
         }
     }
+
+    /// <inheritdoc/>
+    public void Dispose()
+    {
+        SamplingTransactionProfilerFactory factory;
+        lock (_lock)
+        {
+            // Another hub may have registered this integration and still be using the factory.
+            if (_ownedFactory is null || --_registrations > 0)
+            {
+                return;
+            }
+
+            factory = _ownedFactory;
+
+            if (_options is { } options && ReferenceEquals(options.TransactionProfilerFactory, factory))
+            {
+                options.TransactionProfilerFactory = null;
+            }
+
+            _ownedFactory = null;
+            _options = null;
+        }
+
+        factory.Dispose();
+    }
 }

src/Sentry.Profiling/SampleProfilerSession.cs

@@ -47,7 +47,8 @@ private SampleProfilerSession(SentryStopwatch stopwatch, EventPipeSession sessio
     // need a large buffer if we're connecting righ away. Leaving it too large increases app memory usage.
     internal static int CircularBufferMB = 16;
 
-    // Exposed for tests
+    private const int ProcessingDrainTimeoutMs = 2_000;
+
     internal TraceLogEventSource EventSource { get; }
 
     public SampleProfilerTraceEventParser SampleEventParser => _sampleEventParser;
@@ -56,6 +57,12 @@ private SampleProfilerSession(SentryStopwatch stopwatch, EventPipeSession sessio
 
     public TraceLog TraceLog => EventSource.TraceLog;
 
+    internal bool IsStopped => _stopped;
+
+    internal static Action? BeforeStartupForTests;
+
+    internal static Action<SampleProfilerSession>? OnSessionCreatedForTests;
+
     private static InterlockedBoolean _throwOnNextStartupForTests = false;
 
     internal static bool ThrowOnNextStartupForTests
@@ -68,6 +75,8 @@ public static SampleProfilerSession StartNew(IDiagnosticLogger? logger = null)
     {
         try
         {
+            BeforeStartupForTests?.Invoke();
+
             var client = new DiagnosticsClient(Environment.ProcessId);
 
             if (_throwOnNextStartupForTests.CompareExchange(false, true) == true)
@@ -83,16 +92,20 @@ public static SampleProfilerSession StartNew(IDiagnosticLogger? logger = null)
             var eventSource = TraceLog.CreateFromEventPipeSession(session, TraceLog.EventPipeRundownConfiguration.Enable(client));
 
             // Process() blocks until the session is stopped so we need to run it on a separate thread.
+            // The continuation must stay unconditional - one whose criteria aren't met is Canceled,
+            // which would make the Wait() in Stop() throw.
             var processing = Task.Factory.StartNew(eventSource.Process, TaskCreationOptions.LongRunning)
                 .ContinueWith(_ =>
                 {
                     if (_.Exception?.InnerException is { } e)
                     {
                         logger?.LogWarning(e, "Error during sampler profiler EventPipeSession processing.");
                     }
-                }, TaskContinuationOptions.OnlyOnFaulted);
+                });
 
-            return new SampleProfilerSession(stopWatch, session, eventSource, processing, logger);
+            var result = new SampleProfilerSession(stopWatch, session, eventSource, processing, logger);
+            OnSessionCreatedForTests?.Invoke(result);
+            return result;
         }
         catch (Exception ex)
         {
@@ -119,19 +132,37 @@ public async Task WaitForFirstEventAsync(CancellationToken cancellationToken = d
 
     public void Stop()
     {
-        if (!_stopped)
+        if (_stopped)
+        {
+            return;
+        }
+
+        _stopped = true;
+        try
+        {
+            _session.Stop();
+
+            if (!_processing.Wait(ProcessingDrainTimeoutMs))
+            {
+                _logger?.LogWarning("Sampler profiler event processing didn't finish within {0} ms of stopping the session.", ProcessingDrainTimeoutMs);
+            }
+        }
+        catch (Exception ex)
+        {
+            _logger?.LogWarning(ex, "Error during sampler profiler session shutdown.");
+        }
+        finally
         {
+            // These always need to happen, otherwise the EventPipe connection to the 
+            // runtime is left open.
             try
             {
-                _stopped = true;
-                _session.Stop();
-                _processing.Wait();
                 _session.Dispose();
                 EventSource.Dispose();
             }
             catch (Exception ex)
             {
-                _logger?.LogWarning(ex, "Error during sampler profiler session shutdown.");
+                _logger?.LogWarning(ex, "Error disposing the sampler profiler session.");
             }
         }
     }

src/Sentry.Profiling/SamplingTransactionProfilerFactory.cs

@@ -14,23 +14,45 @@ internal class SamplingTransactionProfilerFactory : IDisposable, ITransactionPro
     // Stop profiling after the given number of milliseconds.
     private const int TIME_LIMIT_MS = 30_000;
 
+    private const int SHUTDOWN_TIMEOUT_MS = 2_000;
+
     private readonly SentryOptions _options;
 
     internal Task<SampleProfilerSession> _sessionTask;
 
+    private readonly CancellationTokenSource _shutdownCts = new();
+
+    private readonly object _sessionLock = new();
+    private SampleProfilerSession? _session;
+    private bool _disposed;
+
+    internal bool IsDisposed
+    {
+        get { lock (_sessionLock) { return _disposed; } }
+    }
+
     private bool _errorLogged = false;
 
     public SamplingTransactionProfilerFactory(SentryOptions options, TimeSpan startupTimeout)
     {
         _options = options;
 
+        // Store local reference to avoid ObjectDisposed exception
+        var shutdownToken = _shutdownCts.Token;
+
         _sessionTask = Task.Run(async () =>
         {
             // This can block up to 30 seconds. The timeout is out of our hands.
             var session = SampleProfilerSession.StartNew(options.DiagnosticLogger);
 
+            if (!TryPublishSession(session))
+            {
+                session.Dispose();
+                throw new OperationCanceledException(shutdownToken);
+            }
+
             // This can block indefinitely.
-            await session.WaitForFirstEventAsync().ConfigureAwait(false);
+            await session.WaitForFirstEventAsync(shutdownToken).ConfigureAwait(false);
 
             return session;
         });
@@ -43,9 +65,43 @@ public SamplingTransactionProfilerFactory(SentryOptions options, TimeSpan startu
         }
     }
 
+    private bool TryPublishSession(SampleProfilerSession session)
+    {
+        lock (_sessionLock)
+        {
+            if (_disposed)
+            {
+                return false;
+            }
+
+            _session = session;
+            return true;
+        }
+    }
+
+    private bool TryBeginShutdown(out SampleProfilerSession? sessionToStop)
+    {
+        lock (_sessionLock)
+        {
+            sessionToStop = _session;
+            if (_disposed)
+            {
+                return false;
+            }
+
+            _disposed = true;
+            return true;
+        }
+    }
+
     /// <inheritdoc />
     public ITransactionProfiler? Start(ITransactionTracer _, CancellationToken cancellationToken)
     {
+        if (IsDisposed)
+        {
+            return null;
+        }
+
         // Start a profiler if one wasn't running yet.
         if (!_errorLogged && !_inProgress.Exchange(true))
         {
@@ -83,6 +139,31 @@ public SamplingTransactionProfilerFactory(SentryOptions options, TimeSpan startu
 
     public void Dispose()
     {
-        _sessionTask.ContinueWith(session => session.Dispose());
+        if (!TryBeginShutdown(out var session))
+        {
+            return;
+        }
+
+        _shutdownCts.Cancel();
+
+        try
+        {
+            _sessionTask.Wait(SHUTDOWN_TIMEOUT_MS);
+        }
+        catch (Exception e)
+        {
+            _options.LogDebug("Profiler session didn't start up cleanly before shutdown: {0}", e.Message);
+        }
+
+        try
+        {
+            session?.Dispose();
+        }
+        catch (Exception e)
+        {
+            _options.LogWarning(e, "Failed to stop the profiler session.");
+        }
+
+        _shutdownCts.Dispose();
     }
 }

test/Sentry.Profiling.Tests/ProfilingSentryOptionsExtensionsTests.cs

@@ -61,6 +61,79 @@ public void DisableProfilingIntegration_RemovesProfilingIntegration()
         Assert.DoesNotContain(integrations, i => i is ProfilingIntegration);
     }
 
+    [Fact]
+    public void HubDispose_DisposesTheProfilerFactoryItCreated()
+    {
+        _options.TracesSampleRate = 1.0;
+        _options.ProfilesSampleRate = 1.0;
+
+        var hub = GetSut();
+        var factory = (SamplingTransactionProfilerFactory)_options.TransactionProfilerFactory!;
+        Assert.False(factory.IsDisposed);
+
+        hub.Dispose();
+
+        Assert.True(factory.IsDisposed);
+    }
+
+    [Fact]
+    public void HubDispose_OptionsReusedByANewHub_ProfilerFactoryIsRecreated()
+    {
+        _options.TracesSampleRate = 1.0;
+        _options.ProfilesSampleRate = 1.0;
+
+        using (var first = GetSut())
+        {
+            Assert.NotNull(_options.TransactionProfilerFactory);
+        }
+
+        using var second = GetSut();
+
+        var factory = (SamplingTransactionProfilerFactory)_options.TransactionProfilerFactory!;
+        factory.IsDisposed.Should().BeFalse(
+            "a disposed factory left in the options would silently stop profiling for the new hub");
+    }
+
+    [Fact]
+    public void HubDispose_WhileAnotherHubIsStillRegistered_KeepsTheProfilerFactoryAlive()
+    {
+        _options.TracesSampleRate = 1.0;
+        _options.ProfilesSampleRate = 1.0;
+
+        // SentrySdk.Init is UseHub(InitHub(options)) - the replacement hub registers before the
+        // outgoing one is disposed.
+        var first = GetSut();
+        var factory = (SamplingTransactionProfilerFactory)_options.TransactionProfilerFactory!;
+        var second = GetSut();
+
+        first.Dispose();
+
+        _options.TransactionProfilerFactory.Should().BeSameAs(factory);
+        factory.IsDisposed.Should().BeFalse("the replacement hub is still using it");
+
+        second.Dispose();
+
+        factory.IsDisposed.Should().BeTrue("the last hub using it has gone");
+        _options.TransactionProfilerFactory.Should().BeNull();
+    }
+
+    [Fact]
+    public void HubDispose_DoesNotDisposeAProfilerFactoryItDidNotCreate()
+    {
+        _options.TracesSampleRate = 1.0;
+        _options.ProfilesSampleRate = 1.0;
+
+        var externalFactory = Substitute.For<ITransactionProfilerFactory, IDisposable>();
+        _options.TransactionProfilerFactory = externalFactory;
+
+        using (var hub = GetSut())
+        {
+            Assert.Same(externalFactory, _options.TransactionProfilerFactory);
+        }
+
+        ((IDisposable)externalFactory).DidNotReceive().Dispose();
+    }
+
     [Fact]
     public void AddProfilingIntegration_DoesntDuplicate()
     {