diff --git a/src/Sentry/Internal/BackpressureMonitor.cs b/src/Sentry/Internal/BackpressureMonitor.cs index c45b38264d..1126808ee8 100644 --- a/src/Sentry/Internal/BackpressureMonitor.cs +++ b/src/Sentry/Internal/BackpressureMonitor.cs @@ -36,11 +36,14 @@ internal class BackpressureMonitor : IDisposable private readonly CancellationTokenSource _cts = new(); private readonly Task _workerTask; + private int _disposed; + internal Task WorkerTask => _workerTask; internal int DownsampleLevel => _downsampleLevel; internal long LastQueueOverflowTicks => Interlocked.Read(ref _lastQueueOverflow); internal long LastRateLimitEventTicks => Interlocked.Read(ref _lastRateLimitEvent); - public BackpressureMonitor(IDiagnosticLogger? logger, ISystemClock? clock = null, bool enablePeriodicHealthCheck = true) + public BackpressureMonitor(IDiagnosticLogger? logger, ISystemClock? clock = null, bool enablePeriodicHealthCheck = true, + TaskScheduler? scheduler = null) { _logger = logger; _clock = clock ?? SystemClock.Clock; @@ -48,7 +51,13 @@ public BackpressureMonitor(IDiagnosticLogger? logger, ISystemClock? clock = null if (enablePeriodicHealthCheck) { _logger?.LogDebug("Starting BackpressureMonitor."); - _workerTask = Task.Run(() => DoWorkAsync(_cts.Token)); + // Equivalent to Task.Run(() => DoWorkAsync(_cts.Token)): same creation options (DenyChildAttach) and, + // by default, the same scheduler (the thread pool). Tests inject a scheduler to model single-threaded + // runtimes (e.g. Unity WebGL) where the worker and its continuations must run on the same thread. + scheduler ??= TaskScheduler.Default; + _workerTask = Task.Factory + .StartNew(() => DoWorkAsync(_cts.Token), CancellationToken.None, TaskCreationOptions.DenyChildAttach, scheduler) + .Unwrap(); } else { @@ -144,14 +153,20 @@ internal bool IsHealthy public void Dispose() { - try + // Idempotent and thread-safe: only the first caller runs the disposal logic. Without this guard a + // second call would hit an already-disposed _cts and log a spurious ObjectDisposedException. + if (Interlocked.Exchange(ref _disposed, 1) != 0) { - _cts.Cancel(); - _workerTask.Wait(); + return; } - catch (AggregateException ex) when (ex.InnerException is OperationCanceledException) + + try { - // Ignore cancellation + // Request cancellation but do NOT block on _workerTask here. On single-threaded runtimes + // (e.g. Unity WebGL / browser-wasm) _workerTask.Wait() would cause a deadlock. + // The worker observes the token and unwinds on its own. + // See https://github.com/getsentry/sentry-dotnet/issues/5237 + _cts.Cancel(); } catch (Exception ex) { @@ -160,7 +175,15 @@ public void Dispose() } finally { - _cts.Dispose(); + // Dispose the CTS once the worker has stopped using the token, but + // without blocking the calling thread. Disposing it inline would race the worker + // and could lead to an ObjectDisposedException + _workerTask.ContinueWith( + static (_, state) => ((CancellationTokenSource)state!).Dispose(), + _cts, + CancellationToken.None, + TaskContinuationOptions.ExecuteSynchronously, + TaskScheduler.Default); } } } diff --git a/src/Sentry/Internal/Hub.cs b/src/Sentry/Internal/Hub.cs index dade93b89f..628d0af732 100644 --- a/src/Sentry/Internal/Hub.cs +++ b/src/Sentry/Internal/Hub.cs @@ -943,8 +943,9 @@ public void Dispose() } //Don't dispose of ScopeManager since we want dangling transactions to still be able to access tags. - // Don't dispose of _backpressureMonitor since we want the client to continue to process envelopes without - // throwing an ObjectDisposedException. + // Stop the backpressure monitor's worker so it doesn't outlive the hub... this cancels the + // worker but clients can still read the downsample factor while draining. + _backpressureMonitor?.Dispose(); #if __IOS__ // TODO diff --git a/test/Sentry.Tests/Internals/BackpressureMonitorTests.cs b/test/Sentry.Tests/Internals/BackpressureMonitorTests.cs index 9cb10a4c3d..42e28cd0fc 100644 --- a/test/Sentry.Tests/Internals/BackpressureMonitorTests.cs +++ b/test/Sentry.Tests/Internals/BackpressureMonitorTests.cs @@ -160,4 +160,86 @@ public void DoHealthCheck_Healthy_DownsampleLevelResets() monitor.IsHealthy.Should().BeTrue(); monitor.DownsampleLevel.Should().Be(0); } + + [Fact] + public async Task Dispose_DoesNotBlockOnWorkerTask() + { + // Arrange + // Run the periodic worker on a scheduler we never pump, so the worker task never completes. This models + // a single-threaded runtime (e.g. Unity WebGL) where the only thread able to run the worker's + // cancellation continuation is the one calling Dispose. A blocking _workerTask.Wait() would deadlock + // there; Dispose must instead just cancel and return. See https://github.com/getsentry/sentry-dotnet/issues/5237 + var scheduler = new NeverRunsTaskScheduler(); + var monitor = new BackpressureMonitor(null, _fixture.Clock, enablePeriodicHealthCheck: true, scheduler: scheduler); + + // Act + var disposed = Task.Run(() => monitor.Dispose()); + + // Assert + var completed = await Task.WhenAny(disposed, Task.Delay(TimeSpan.FromSeconds(10))); + completed.Should().BeSameAs(disposed, "Dispose must not block waiting on the worker task to complete"); + await disposed; // surface any exception thrown by Dispose + } + + [Fact] + public async Task Dispose_WorkerRunsToCompletionWithoutFaulting() + { + // Arrange - a real worker on the thread pool. Dispose cancels the token while the worker may still be + // inside Task.Delay; the CancellationTokenSource must not be disposed out from under it (which would + // surface an unobserved ObjectDisposedException). See https://github.com/getsentry/sentry-dotnet/issues/5237 + _fixture.Clock.GetUtcNow().Returns(_fixture.Now); + var monitor = new BackpressureMonitor(null, _fixture.Clock, enablePeriodicHealthCheck: true); + var worker = monitor.WorkerTask; + + // Act + monitor.Dispose(); + await worker; // observes the task; throws if it faulted + + // Assert + worker.Status.Should().Be(TaskStatus.RanToCompletion); + } + + [Fact] + public void Dispose_CalledMultipleTimes_IsIdempotent() + { + // Arrange + var logger = Substitute.For(); + _fixture.Clock.GetUtcNow().Returns(_fixture.Now); + var monitor = new BackpressureMonitor(logger, _fixture.Clock, enablePeriodicHealthCheck: false); + + // Act + monitor.Dispose(); + monitor.Dispose(); + monitor.Dispose(); + + // Assert - the second and third calls are no-ops; no ObjectDisposedException is logged. + logger.DidNotReceive().Log( + SentryLevel.Warning, Arg.Any(), Arg.Any(), Arg.Any()); + } + + /// + /// A scheduler that queues tasks but never executes them - lets us hold the worker task in a state that + /// never completes, so a Dispose that blocks on it would hang. + /// + private sealed class NeverRunsTaskScheduler : TaskScheduler + { + private readonly List _tasks = new(); + protected override IEnumerable GetScheduledTasks() + { + lock (_tasks) + { + return _tasks.ToArray(); + } + } + + protected override void QueueTask(Task task) + { + lock (_tasks) + { + _tasks.Add(task); + } + } + + protected override bool TryExecuteTaskInline(Task task, bool taskWasPreviouslyQueued) => false; + } }