diff --git a/RunCommand.Test/RunCommandTests.cs b/RunCommand.Test/RunCommandTests.cs index 8fb5335..03fd4c4 100644 --- a/RunCommand.Test/RunCommandTests.cs +++ b/RunCommand.Test/RunCommandTests.cs @@ -468,6 +468,44 @@ await Assert.ThrowsAsync( } } + [TestMethod] + public async Task ExecuteAsyncShouldReturnWhenCancelledWhileADetachedDescendantHoldsTheOutputPipe() + { + if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + { + Assert.Inconclusive("Needs a shell that can orphan a child out of its own process tree while that child keeps the pipe it inherited. The wait this covers is in platform independent code, so the other legs cover it."); + } + + using CancellationTokenSource cancellationTokenSource = new(); + + // The inner shell backgrounds a sleep and exits immediately, so that sleep is reparented to + // init and is no longer a descendant the entire-process-tree kill can walk to — but it still + // holds the standard output and standard error handles it inherited. The outer sleep keeps + // the process this call owns alive, so cancellation is what ends it. Killing that process + // therefore closes neither pipe's write end, and end of stream never arrives. + // + // setsid is not enough here: it gives the child its own session but leaves its parent alone, + // so the kill still reaches it. + Task execution = RunCommand.ExecuteAsync( + "sh", + ["-c", "sh -c 'sleep 30 &'; sleep 30"], + new OutputHandler(), + cancellationTokenSource.Token); + + await cancellationTokenSource.CancelAsync().ConfigureAwait(false); + + // Bounded rather than a bare await: before the fix this call never returns, and a test that + // hangs takes the whole run down with it instead of reporting a failure. + Task finished = await Task.WhenAny(execution, Task.Delay(TimeSpan.FromSeconds(10))).ConfigureAwait(false); + + Assert.AreSame( + execution, + finished, + "Expected a cancelled call to return promptly rather than wait on a pipe an orphaned descendant still holds open."); + + await Assert.ThrowsAsync(() => execution).ConfigureAwait(false); + } + [TestMethod] public async Task ExecuteAsyncShouldStartTheProcessInTheGivenWorkingDirectory() { diff --git a/RunCommand/AsyncProcessStreamReader.cs b/RunCommand/AsyncProcessStreamReader.cs index 1a8c809..c04be15 100644 --- a/RunCommand/AsyncProcessStreamReader.cs +++ b/RunCommand/AsyncProcessStreamReader.cs @@ -3,6 +3,7 @@ namespace ktsu.RunCommand; using System.Diagnostics; +using System.Threading; internal sealed class AsyncProcessStreamReader(Process process, OutputHandler outputHandler) : IDisposable { @@ -32,8 +33,34 @@ public void Dispose() errorStream.Dispose(); } - internal async Task Start() + /// + /// Pumps the process's standard output and standard error to the handler until both pipes reach + /// end of stream, or until is signalled. + /// + /// + /// A read of a redirected pipe returns on new data or on end of stream, and nothing else — there + /// is no token that reaches into it, and a pipe read is not reliably interruptible even on the + /// targets whose offers a cancellable overload. So cancellation is + /// handled by giving up on the pending read rather than by cancelling it. + /// + /// That distinction is the whole point. End of stream means every handle on the write end has + /// closed, and killing the command closes only the handles the command itself held: one that a + /// descendant inherited and carried past its parent's death keeps the pipe open. Waiting for end + /// of stream after a kill therefore waits on something that may never happen, which left a + /// cancelled call hanging indefinitely. + /// + /// + /// The token the caller cancelled the run with. + internal async Task Start(CancellationToken cancellationToken) { + TaskCompletionSource cancellationSource = new(); + + using CancellationTokenRegistration registration = cancellationToken.Register( + static state => ((TaskCompletionSource)state!).TrySetResult(true), + cancellationSource); + + Task cancelled = cancellationSource.Task; + Task outputTask = Task.CompletedTask; Task errorTask = Task.CompletedTask; @@ -59,16 +86,69 @@ internal async Task Start() errorTask = ReadAndCallback(errorStream, errorBuffer, outputHandler.HandleStandardErrorData, isStandardOutput: false); } - await Task.WhenAny(outputTask, errorTask).ConfigureAwait(false); + Task first = await Task.WhenAny(outputTask, errorTask, cancelled).ConfigureAwait(false); + if (ReferenceEquals(first, cancelled)) + { + Abandon(outputTask, errorTask); + return; + } } while (!process.HasExited); - await Task.WhenAll(outputTask, errorTask).ConfigureAwait(false); + if (!await DrainOrAbandon(outputTask, errorTask, cancelled).ConfigureAwait(false)) + { + return; + } // Read any remaining data after process exit. outputTask = ReadAndCallback(outputStream, outputBuffer, outputHandler.HandleStandardOutputData, isStandardOutput: true); errorTask = ReadAndCallback(errorStream, errorBuffer, outputHandler.HandleStandardErrorData, isStandardOutput: false); - await Task.WhenAll(outputTask, errorTask).ConfigureAwait(false); + _ = await DrainOrAbandon(outputTask, errorTask, cancelled).ConfigureAwait(false); + } + + /// + /// Waits for both reads to finish, unless cancellation gets there first. + /// + /// + /// when both reads finished, so the caller may carry on; + /// when cancellation won and the reads were abandoned. + /// + private static async Task DrainOrAbandon(Task outputTask, Task errorTask, Task cancelled) + { + Task reads = Task.WhenAll(outputTask, errorTask); + + if (ReferenceEquals(await Task.WhenAny(reads, cancelled).ConfigureAwait(false), cancelled)) + { + Abandon(outputTask, errorTask); + return false; + } + + // Awaited rather than returned so that a read that failed still throws here, which is what + // carries a decode error out to the caller. + await reads.ConfigureAwait(false); + return true; + } + + /// + /// Leaves reads this call has given up on to end however they end, observing the result. + /// + /// + /// Disposing the readers ends an abandoned read, but it ends by faulting, and a faulted task + /// nobody ever looks at raises when it is + /// finalized. Looking at it here keeps a cancelled run from tripping that on an unrelated thread + /// later. A read that never ends at all costs a buffer until the process handle is released, + /// which is the price of not waiting on a pipe the caller has already walked away from. + /// + private static void Abandon(params Task[] reads) + { + foreach (Task read in reads) + { + _ = read.ContinueWith( + static abandoned => _ = abandoned.Exception, + CancellationToken.None, + TaskContinuationOptions.OnlyOnFaulted | TaskContinuationOptions.ExecuteSynchronously, + TaskScheduler.Default); + } } private async Task ReadAndCallback(StreamReader streamReader, char[] buffer, Action? onData, bool isStandardOutput) => diff --git a/RunCommand/RunCommand.cs b/RunCommand/RunCommand.cs index bcf37bb..731bf16 100644 --- a/RunCommand/RunCommand.cs +++ b/RunCommand/RunCommand.cs @@ -463,7 +463,7 @@ private static async Task RunAsync(ProcessStartInfo startInfo, OutputHandle else { using AsyncProcessStreamReader outputReader = new(process, outputHandler); - await Task.WhenAll(outputReader.Start(), process.WaitForExitAsync(cancellationToken)).ConfigureAwait(false); + await Task.WhenAll(outputReader.Start(cancellationToken), process.WaitForExitAsync(cancellationToken)).ConfigureAwait(false); } // Cancellation reaches the wait two ways at once: the registration above kills the process,