diff --git a/RunCommand.Test/RunCommandTests.cs b/RunCommand.Test/RunCommandTests.cs index 3a34051..97c1191 100644 --- a/RunCommand.Test/RunCommandTests.cs +++ b/RunCommand.Test/RunCommandTests.cs @@ -471,6 +471,91 @@ await Assert.ThrowsAsync( } } + [TestMethod] + public async Task ExecuteAsyncShouldKillTheProcessTreeEveryTimeItIsCancelled() + { + if (!RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) + { + Assert.Inconclusive("Reads a descendant's state from /proc. The kill this covers is in platform independent code, so the Linux leg covers it."); + } + + // A token runs its callbacks newest first, and the wait registers its own after the kill's. + // So the wait can observe cancellation, end the call, and dispose the kill's registration + // before that callback has run, and the call returns with its whole tree still running. The + // wait only wins that race some of the time, which is why this repeats. The shape is a + // caller's own deadline linked into its token, which cancels on a timer thread. + for (int attempt = 0; attempt < 40; attempt++) + { + TaskCompletionSource descendant = new(TaskCreationOptions.RunContinuationsAsynchronously); + OutputHandler handler = new( + chunk => + { + if (int.TryParse(chunk.Trim(), out int id)) + { + descendant.TrySetResult(id); + } + }, + _ => { }); + + using CancellationTokenSource timeout = new(TimeSpan.FromMilliseconds(200)); + using CancellationTokenSource linked = CancellationTokenSource.CreateLinkedTokenSource(CancellationToken.None, timeout.Token); + + Task execution = RunCommand.ExecuteAsync( + "sh", + ["-c", "sleep 30 & echo $!; wait"], + handler, + new CommandOptions { StandardInput = StandardInputMode.Closed }, + linked.Token); + + int descendantId = await descendant.Task.WaitAsync(TimeSpan.FromSeconds(10)).ConfigureAwait(false); + + await Assert.ThrowsAsync(() => execution).ConfigureAwait(false); + + Assert.IsTrue( + await HasExitedAsync(descendantId).ConfigureAwait(false), + $"Attempt {attempt}: descendant {descendantId} was still running after its call was cancelled."); + } + } + + /// + /// Waits up to five seconds for a process to exit. + /// + /// + /// Reads /proc rather than asking , because the process is not a child + /// of this one: once its parent is killed it is reparented, and it can sit as a zombie until its + /// new parent reaps it. A zombie has exited, so it counts as gone. + /// + private static async Task HasExitedAsync(int processId) + { + for (int check = 0; check < 50; check++) + { + string status; + + try + { + status = await File.ReadAllTextAsync($"/proc/{processId}/stat").ConfigureAwait(false); + } + catch (IOException) + { + return true; + } + catch (UnauthorizedAccessException) + { + return true; + } + + // The state follows the parenthesised command name, which may itself contain spaces. + if (status[(status.LastIndexOf(')') + 2)..].StartsWith('Z')) + { + return true; + } + + await Task.Delay(100).ConfigureAwait(false); + } + + return false; + } + [TestMethod] public async Task ExecuteAsyncShouldReturnWhenCancelledWhileADetachedDescendantHoldsTheOutputPipe() { diff --git a/RunCommand/RunCommand.cs b/RunCommand/RunCommand.cs index e3da840..0bc9b75 100644 --- a/RunCommand/RunCommand.cs +++ b/RunCommand/RunCommand.cs @@ -475,14 +475,27 @@ private static async Task RunAsync(ProcessStartInfo startInfo, OutputHandle // would leave the child running unsupervised. using CancellationTokenRegistration registration = cancellationToken.Register(() => TryKill(process)); - if (useElevation) + try { - await process.WaitForExitAsync(cancellationToken).ConfigureAwait(false); + if (useElevation) + { + await process.WaitForExitAsync(cancellationToken).ConfigureAwait(false); + } + else + { + using AsyncProcessStreamReader outputReader = new(process, outputHandler); + await Task.WhenAll(outputReader.Start(cancellationToken), process.WaitForExitAsync(cancellationToken)).ConfigureAwait(false); + } } - else + catch (OperationCanceledException) { - using AsyncProcessStreamReader outputReader = new(process, outputHandler); - await Task.WhenAll(outputReader.Start(cancellationToken), process.WaitForExitAsync(cancellationToken)).ConfigureAwait(false); + // The registration alone does not guarantee the kill. A token runs its callbacks newest + // first, and the wait registered its own after this one, so the wait can end this call + // and dispose the registration before the kill callback has run. That leaves the whole + // tree running unsupervised. Killing here as well makes it certain; a second kill of a + // process that has already gone is a no-op. + TryKill(process); + throw; } // Cancellation reaches the wait two ways at once: the registration above kills the process,