Skip to content

fix: BackpressureMonitor.Dispose() no longer deadlocks on single-threaded targets#5330

Open
jamescrosswell wants to merge 7 commits into
mainfrom
fix/5237-backpressure-dispose-deadlock
Open

fix: BackpressureMonitor.Dispose() no longer deadlocks on single-threaded targets#5330
jamescrosswell wants to merge 7 commits into
mainfrom
fix/5237-backpressure-dispose-deadlock

Conversation

@jamescrosswell

@jamescrosswell jamescrosswell commented Jun 28, 2026

Copy link
Copy Markdown
Collaborator

Fixes #5237

Problem

On single-threaded runtimes (Unity WebGL / browser-wasm) there is no separate thread pool — Task.Run and Task.Delay continuations all run on the one main thread. BackpressureMonitor.Dispose() did:

_cts.Cancel();
_workerTask.Wait();   // blocks the only thread

_workerTask.Wait() synchronously blocks the calling thread waiting for the worker's post-cancellation continuation, which on a single-threaded runtime can only be scheduled on that same blocked thread (e.g. calling unityInstance.Quit() in Unity WebGL).

ConfigureAwait(false) does not help here. On a desktop host the cancellation continuation escapes to a real thread-pool thread. On single-threaded targets the "thread pool" is the main thread, so the continuation has nowhere to run except the thread that is blocked in Wait().

Fix

Dispose() now only requests cancellation and returns — it does not block on the worker. The worker observes the token, exits its Task.Delay loop and unwinds on its own; it produces no result anyone needs to await.

Because the monitor's public methods (DownsampleFactor / RecordQueueOverflow / RecordRateLimitHit) never touch _cts, it is now safe to dispose the monitor even while the client keeps draining envelopes. This lets Hub.Dispose() dispose the monitor again rather than leaking the worker for the process lifetime.

⚠️ Hub.Dispose()

The call to _backpressureMonitor?.Dispose() was removed in #4613 with the comment:

Don't dispose … without throwing an ObjectDisposedException

I've restored it because, with a non-blocking Dispose() that only cancels/disposes the CTS, none of the monitor's post-disposal call sites touch the disposed token source — so the original ObjectDisposedException should not recur.

Warning

TODO: Confirm this matches the scenario #4613 was guarding against. We can always reinstate the fix from #4613 if we're concerned about this.

Affected versions

The live deadlock path (Hub.Dispose()_backpressureMonitor?.Dispose()) exists in 4.x/5.x (and the Sentry Unity 4.x that reported this). It was incidentally removed on main/6.0.0+ by #4613, so 6.x no longer reaches the deadlock - but at the cost of leaking the worker. Dispose() itself remained latently broken. This PR fixes the root cause so the call is safe again. Users on ≤5.x operating in a single threaded environment should set EnableBackpressureHandling = false as a workaround instead.

🤖 Generated with Claude Code

…aded targets

On single-threaded runtimes (Unity WebGL / browser-wasm) there is no separate
thread pool: Task.Run and Task.Delay continuations all run on the one main
thread. BackpressureMonitor.Dispose() called _cts.Cancel() followed by a
synchronous _workerTask.Wait(), which blocked the only thread that could run
the worker's cancellation continuation - a deadlock. On Unity WebGL this wedged
unityInstance.Quit() indefinitely (#5237).

Dispose() now only requests cancellation and returns; the worker observes the
token and unwinds on its own. The monitor's public methods (DownsampleFactor /
RecordQueueOverflow / RecordRateLimitHit) never touch the cancellation token
source, so it is now safe for Hub.Dispose() to dispose the monitor again rather
than leaking the worker for the lifetime of the process.

Adds a regression test that runs the worker on a scheduler that never completes
and asserts Dispose() returns without blocking.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@codecov

codecov Bot commented Jun 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 74.20%. Comparing base (0061920) to head (a56d248).

Files with missing lines Patch % Lines
src/Sentry/Internal/BackpressureMonitor.cs 93.75% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5330      +/-   ##
==========================================
+ Coverage   74.16%   74.20%   +0.04%     
==========================================
  Files         508      508              
  Lines       18353    18363      +10     
  Branches     3586     3589       +3     
==========================================
+ Hits        13612    13627      +15     
+ Misses       3869     3864       -5     
  Partials      872      872              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread src/Sentry/Internal/BackpressureMonitor.cs Outdated
Comment thread src/Sentry/Internal/Hub.cs Outdated
Co-authored-by: James Crosswell <jamescrosswell@users.noreply.github.com>
@jamescrosswell jamescrosswell marked this pull request as ready for review June 29, 2026 23:53
Comment thread src/Sentry/Internal/BackpressureMonitor.cs
jamescrosswell and others added 2 commits June 30, 2026 12:05
…rker stops

Removing the blocking _workerTask.Wait() from Dispose() (to avoid the
single-threaded deadlock) reintroduced a race: the inline _cts.Dispose() could
run while the worker was still inside Task.Delay(..., token) registering its
continuation. On platforms where registering on a disposed source throws (e.g.
.NET Framework) the worker would observe an ObjectDisposedException - which it
doesn't catch - and fault with an unobserved task exception.

Dispose the CancellationTokenSource from a non-blocking continuation on the
worker task instead, so it is only disposed once the worker has stopped using
the token. Adds a test asserting the worker runs to completion without faulting
after Dispose.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…adlock' into fix/5237-backpressure-dispose-deadlock
Comment thread src/Sentry/Internal/BackpressureMonitor.cs
Comment thread src/Sentry/Internal/BackpressureMonitor.cs Outdated
jamescrosswell and others added 3 commits June 30, 2026 12:19
A second Dispose() call would hit the already-disposed _cts and log a spurious
ObjectDisposedException warning (and schedule redundant cleanup). Guard with a
thread-safe Interlocked flag so only the first caller runs the disposal logic.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…etsentry/sentry-dotnet into fix/5237-backpressure-dispose-deadlock
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BackpressureMonitor.Dispose() deadlocks on single-threaded targets (Unity WebGL)

1 participant