Skip to content

Commit 8f5962b

Browse files
kblokclaude
andcommitted
fix: prevent hang in TaskQueue.Dispose when closing connections with pending messages
The tab target model (upstream #14821) causes Chrome to emit many Target.attachedToTarget events; each is processed via an async void Transport_MessageReceived that calls _callbackQueue.Enqueue, queueing many SemaphoreSlim.WaitAsync callers. The old TaskQueue.Dispose called _semaphore.Wait() synchronously, blocking until every pending waiter drained — which never happened while the Connection was still receiving events, causing a 40+ second hang in ShouldWorkAcrossSessions. Fix by adding a CancellationTokenSource to TaskQueue: Dispose() cancels it, causing all pending WaitAsync(_disposeCts.Token) callers to receive OperationCanceledException and exit immediately, so Dispose returns without blocking. Also included: - Idempotent Connection.Dispose() guard to prevent double-dispose - Session/callback cleanup moved before Disconnected fires to avoid a deadlock where the Disconnected handler awaits a session response that will never arrive - CdpCDPSession re-checks Detached after inserting into _callbacks to close a race window where Close() could miss the new entry Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 4c57a4f commit 8f5962b

3 files changed

Lines changed: 46 additions & 8 deletions

File tree

lib/PuppeteerSharp/Cdp/CdpCDPSession.cs

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,19 @@ public override async Task DetachAsync()
103103
Message = message,
104104
};
105105
_callbacks[id] = callback;
106+
107+
// Re-check Detached after adding to _callbacks. This closes the race where
108+
// Close() runs between our initial Detached check and the dictionary insertion,
109+
// leaving our callback uncancelled so the response wait never resolves.
110+
if (Detached)
111+
{
112+
_callbacks.TryRemove(id, out _);
113+
throw new TargetClosedException(
114+
$"Protocol error ({method}): Session closed. " +
115+
$"Most likely the {_targetType} has been closed." +
116+
$"Close reason: {CloseReason ?? Connection.CloseReason}",
117+
CloseReason ?? Connection.CloseReason);
118+
}
106119
}
107120

108121
try

lib/PuppeteerSharp/Cdp/Connection.cs

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ public sealed class Connection : IDisposable, ICDPConnection
2828
private readonly AsyncDictionaryHelper<string, CdpCDPSession> _sessions = new("Session {0} not found");
2929
private readonly List<string> _manuallyAttached = [];
3030
private int _lastId;
31+
private int _disposed;
3132

3233
private Connection(string url, int delay, bool enqueueAsyncMessages, IConnectionTransport transport, ILoggerFactory loggerFactory = null, int protocolTimeout = DefaultCommandTimeout)
3334
{
@@ -102,6 +103,11 @@ private Connection(string url, int delay, bool enqueueAsyncMessages, IConnection
102103
/// <inheritdoc />
103104
public void Dispose()
104105
{
106+
if (Interlocked.Exchange(ref _disposed, 1) != 0)
107+
{
108+
return;
109+
}
110+
105111
Dispose(true);
106112
GC.SuppressFinalize(this);
107113
}
@@ -223,8 +229,11 @@ internal void Close(string closeReason)
223229
CloseReason = closeReason;
224230

225231
Transport.StopReading();
226-
Disconnected?.Invoke(this, EventArgs.Empty);
227232

233+
// Cancel all pending session commands and callbacks before firing Disconnected.
234+
// This prevents a deadlock where the Disconnected handler (via CloseCoreAsync)
235+
// tries to dispose the callback queue while a queued message is awaiting a
236+
// session response that will never arrive (because session cleanup hasn't run yet).
228237
foreach (var session in _sessions.Values)
229238
{
230239
session.Close(closeReason);
@@ -241,6 +250,8 @@ internal void Close(string closeReason)
241250

242251
_callbacks.Clear();
243252
MessageQueue.Dispose();
253+
254+
Disconnected?.Invoke(this, EventArgs.Empty);
244255
}
245256

246257
internal CdpCDPSession GetSession(string sessionId) => _sessions.GetValueOrDefault(sessionId);

lib/PuppeteerSharp/Helpers/TaskQueue.cs

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ internal sealed class TaskQueue : IDisposable, IAsyncDisposable
88
{
99
private readonly SemaphoreSlim _semaphore;
1010
private readonly AsyncLocal<bool> _held = new();
11+
private readonly CancellationTokenSource _disposeCts = new();
1112
private int _disposed;
1213

1314
internal TaskQueue() => _semaphore = new SemaphoreSlim(1);
@@ -19,11 +20,8 @@ public void Dispose()
1920
return;
2021
}
2122

22-
if (!_held.Value)
23-
{
24-
_semaphore.Wait();
25-
}
26-
23+
_disposeCts.Cancel();
24+
_disposeCts.Dispose();
2725
_semaphore.Dispose();
2826
}
2927

@@ -44,7 +42,15 @@ public async ValueTask DisposeAsync()
4442

4543
internal async Task<T> Enqueue<T>(Func<Task<T>> taskGenerator)
4644
{
47-
await _semaphore.WaitAsync().ConfigureAwait(false);
45+
try
46+
{
47+
await _semaphore.WaitAsync(_disposeCts.Token).ConfigureAwait(false);
48+
}
49+
catch (OperationCanceledException)
50+
{
51+
return default;
52+
}
53+
4854
try
4955
{
5056
_held.Value = true;
@@ -59,7 +65,15 @@ internal async Task<T> Enqueue<T>(Func<Task<T>> taskGenerator)
5965

6066
internal async Task Enqueue(Func<Task> taskGenerator)
6167
{
62-
await _semaphore.WaitAsync().ConfigureAwait(false);
68+
try
69+
{
70+
await _semaphore.WaitAsync(_disposeCts.Token).ConfigureAwait(false);
71+
}
72+
catch (OperationCanceledException)
73+
{
74+
return;
75+
}
76+
6377
try
6478
{
6579
_held.Value = true;

0 commit comments

Comments
 (0)