From 53c283837a10026e315e754a776fcbbf2c177789 Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Tue, 6 Oct 2026 03:26:44 +0000 Subject: [PATCH] [patch] Run continuations of the cancellation signal asynchronously, so Cancel() returns before the unwind The reader's cancellation TaskCompletionSource was completed from inside CancellationTokenSource.Cancel() without RunContinuationsAsynchronously, so the whole unwind ran on the cancelling thread before Cancel() returned: the read drain, the process-tree kill, and the caller's own catch block. A caller that cancelled while holding a lock its cleanup also takes deadlocked. Fixes #103 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Bm33TiYrofqKs4oGc3UGUU --- RunCommand.Test/RunCommandTests.cs | 85 ++++++++++++++++++++++++++ RunCommand/AsyncProcessStreamReader.cs | 7 ++- 2 files changed, 91 insertions(+), 1 deletion(-) diff --git a/RunCommand.Test/RunCommandTests.cs b/RunCommand.Test/RunCommandTests.cs index 818e08c..5003004 100644 --- a/RunCommand.Test/RunCommandTests.cs +++ b/RunCommand.Test/RunCommandTests.cs @@ -450,6 +450,91 @@ public async Task ExecuteAsyncShouldTerminateProcessWhenCancelledWhileRunning() await Assert.ThrowsAsync(() => execution).ConfigureAwait(false); } + [TestMethod] + public async Task CancelShouldNotRunTheCallersContinuationInline() + { + using CancellationTokenSource cancellationTokenSource = new(); + (string fileName, string[] arguments) = GetSleepCommand(); + bool insideCancel = false; + bool continuationRanInsideCancel = false; + + Task execution = Task.Run(async () => + { + try + { + _ = await RunCommand.ExecuteAsync(fileName, arguments, new OutputHandler(), cancellationTokenSource.Token).ConfigureAwait(false); + } + catch (OperationCanceledException) + { + continuationRanInsideCancel = Volatile.Read(ref insideCancel); + } + }); + + // Give the command time to start, so the cancellation reaches a run that is reading output. + await Task.Delay(TimeSpan.FromMilliseconds(500)).ConfigureAwait(false); + + Volatile.Write(ref insideCancel, true); + CancelSynchronously(cancellationTokenSource); + Volatile.Write(ref insideCancel, false); + + Task finished = await Task.WhenAny(execution, Task.Delay(TimeSpan.FromSeconds(10))).ConfigureAwait(false); + Assert.AreSame(execution, finished, "Expected the cancelled call to end."); + Assert.IsFalse(continuationRanInsideCancel, "Expected the caller's catch block to run after Cancel() returned, not inside it."); + } + + [TestMethod] + public async Task CancelWhileHoldingALockTheCallersCleanupNeedsShouldNotDeadlock() + { + using CancellationTokenSource cancellationTokenSource = new(); + using SemaphoreSlim gate = new(1, 1); + (string fileName, string[] arguments) = GetSleepCommand(); + bool cleanupAcquiredTheLock = false; + + Task execution = Task.Run(async () => + { + try + { + _ = await RunCommand.ExecuteAsync(fileName, arguments, new OutputHandler(), cancellationTokenSource.Token).ConfigureAwait(false); + } + catch (OperationCanceledException) + { + // Bounded so that, before the fix, this reports a failure instead of hanging the run: + // the cleanup ran inside Cancel(), on the thread that holds the gate. + cleanupAcquiredTheLock = WaitSynchronously(gate, TimeSpan.FromSeconds(5)); + if (cleanupAcquiredTheLock) + { + _ = gate.Release(); + } + } + }); + + await Task.Delay(TimeSpan.FromMilliseconds(500)).ConfigureAwait(false); + + await gate.WaitAsync().ConfigureAwait(false); + Stopwatch cancelTime = Stopwatch.StartNew(); + try + { + CancelSynchronously(cancellationTokenSource); + } + finally + { + cancelTime.Stop(); + _ = gate.Release(); + } + + Task finished = await Task.WhenAny(execution, Task.Delay(TimeSpan.FromSeconds(15))).ConfigureAwait(false); + Assert.AreSame(execution, finished, "Expected the cancelled call to end."); + Assert.IsTrue(cleanupAcquiredTheLock, $"Expected the caller's cleanup to acquire the lock once Cancel() released it; Cancel() took {cancelTime.ElapsedMilliseconds} ms."); + } + + // Cancel() rather than CancelAsync(), because the difference under test is what runs on the + // cancelling thread before Cancel() returns. + private static void CancelSynchronously(CancellationTokenSource cancellationTokenSource) => + cancellationTokenSource.Cancel(); + + private static bool WaitSynchronously(SemaphoreSlim semaphore, TimeSpan timeout) => + semaphore.Wait(timeout); + [TestMethod] public async Task ExecuteAsyncShouldThrowRatherThanReturnAnExitCodeWhenCancellationWinsTheRace() { diff --git a/RunCommand/AsyncProcessStreamReader.cs b/RunCommand/AsyncProcessStreamReader.cs index e7d437e..9f0c80e 100644 --- a/RunCommand/AsyncProcessStreamReader.cs +++ b/RunCommand/AsyncProcessStreamReader.cs @@ -53,7 +53,12 @@ public void Dispose() /// The token the caller cancelled the run with. internal async Task Start(CancellationToken cancellationToken) { - TaskCompletionSource cancellationSource = new(); + // Continuations run asynchronously because the token's registration completes this source + // from inside CancellationTokenSource.Cancel(). Run synchronously, they took the whole + // unwind with them onto the cancelling thread, process-tree kill and the caller's own catch + // block included, before Cancel() returned. A caller cancelling while holding a lock its + // cleanup also needs then deadlocked. + TaskCompletionSource cancellationSource = new(TaskCreationOptions.RunContinuationsAsynchronously); using CancellationTokenRegistration registration = cancellationToken.Register( static state => ((TaskCompletionSource)state!).TrySetResult(true),