diff --git a/CLAUDE.md b/CLAUDE.md index ca78b82..0d9bdfa 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -46,7 +46,7 @@ output through delegates. The solution uses: - **ktsu.Sdk** - Custom SDK providing shared build configuration - **MSTest.Sdk** - Test project SDK with Microsoft Testing Platform -- Multi-targeting: `net10.0`, `net9.0`, `net8.0`, `net7.0`, `net6.0`, `net5.0`, `netstandard2.0`, `netstandard2.1` +- Multi-targeting: `net10.0`, `net9.0`, `net8.0`, `netstandard2.0`, `netstandard2.1` (set by ktsu.Sdk). Apps on .NET Core 3.x and .NET 5–7 load the `netstandard2.1` build The test project targets `net10.0` only. @@ -109,8 +109,11 @@ before returning. **Do not remove this** — without it a cancelled command is i a genuine failure of the underlying tool. The regression test repeats a 1 ms cancellation 50 times, because a single attempt still throws most of the time even when the bug is present. -Process-tree termination requires .NET Core 3.0 or later; the `netstandard2.0`/`netstandard2.1` -builds can only kill the process itself. +Process-tree termination requires .NET Core 3.0 or later. The `netstandard2.0`/`netstandard2.1` builds +find `Process.Kill(bool)` by reflection in `KillEntireTreeWhereSupported`, because those builds are +what .NET Core 3.x and .NET 5–7 apps load, and fall back to killing the process alone on .NET +Framework and Mono. The reflection call unwraps `TargetInvocationException` so `TryKill` still sees +the overload's own exceptions. ### Elevation constraints diff --git a/README.md b/README.md index d86b895..b30ec9f 100644 --- a/README.md +++ b/README.md @@ -179,7 +179,7 @@ class Program A cancelled call always throws `OperationCanceledException` — it never returns the killed process's exit code — so cancellation cannot be mistaken for a genuine failure of the command. -On .NET Core 3.0 and later the entire process tree is terminated. On .NET Standard 2.0 and 2.1 only the process itself can be terminated, so any grandchildren it spawned are left running. +On .NET Core 3.0 and later, including apps on .NET Core 3.x and .NET 5–7 that load the .NET Standard build, the entire process tree is terminated. On .NET Framework and Mono only the process itself can be terminated, so any grandchildren it spawned are left running. ## Process Options diff --git a/RunCommand.Test/RunCommandTests.cs b/RunCommand.Test/RunCommandTests.cs index 3a202a5..aa7261f 100644 --- a/RunCommand.Test/RunCommandTests.cs +++ b/RunCommand.Test/RunCommandTests.cs @@ -488,6 +488,84 @@ static void PartlyFailingKill(Process _) => } } + [TestMethod] + public void KillEntireTreeWhereSupportedShouldEndTheGrandchildrenToo() + { + SkipOnWindows(); + + // The netstandard builds, which .NET Core 3.x and .NET 5–7 apps load, kill through this method. Killing only + // the shell would leave its background sleep running (#134). + using Process process = Process.Start(new ProcessStartInfo("sh", ["-c", "sleep 300 & echo $!; wait"]) + { + UseShellExecute = false, + RedirectStandardOutput = true, + })!; + int grandchild = int.Parse(process.StandardOutput.ReadLine()!, System.Globalization.CultureInfo.InvariantCulture); + + try + { + RunCommand.KillEntireTreeWhereSupported(process); + process.WaitForExit(); + + bool ended = SpinWait.SpinUntil(() => !IsRunning(grandchild), TimeSpan.FromSeconds(5)); + + Assert.IsTrue(ended, "Expected the shell's background sleep to be killed with it."); + } + finally + { + if (IsRunning(grandchild)) + { + using Process leftover = Process.GetProcessById(grandchild); + leftover.Kill(); + } + } + + static bool IsRunning(int processId) + { + try + { + using Process candidate = Process.GetProcessById(processId); + return !candidate.HasExited; + } + catch (ArgumentException) + { + return false; + } + } + } + + [TestMethod] + public void KillEntireTreeWhereSupportedShouldKillTheProcessAloneWithoutTheTreeOverload() + { + // .NET Framework and Mono have no Process.Kill(bool), so the process itself is still killed there. + (string fileName, string[] arguments) = GetSleepCommand(); + using Process process = Process.Start(new ProcessStartInfo(fileName, arguments) { UseShellExecute = false })!; + + try + { + RunCommand.KillEntireTreeWhereSupported(process, killEntireProcessTree: null); + + Assert.IsTrue(process.WaitForExit(10_000), "Expected the process to be killed without the tree overload."); + } + finally + { + if (!process.HasExited) + { + process.Kill(entireProcessTree: true); + } + } + } + + [TestMethod] + public void KillEntireTreeWhereSupportedShouldThrowTheOverloadsOwnExceptionUnwrapped() + { + // TryKill catches InvalidOperationException, Win32Exception and AggregateException by type, so a + // TargetInvocationException from the reflection call would escape it (#107). + using Process neverStarted = new(); + + _ = Assert.ThrowsExactly(() => RunCommand.KillEntireTreeWhereSupported(neverStarted)); + } + [TestMethod] public async Task CancelShouldNotRunTheCallersContinuationInline() { diff --git a/RunCommand/RunCommand.cs b/RunCommand/RunCommand.cs index 2e538d5..57211b6 100644 --- a/RunCommand/RunCommand.cs +++ b/RunCommand/RunCommand.cs @@ -6,6 +6,8 @@ namespace ktsu.RunCommand; using System.Collections.Generic; using System.Diagnostics; using System.IO; +using System.Reflection; +using System.Runtime.ExceptionServices; using System.Runtime.InteropServices; using System.Threading; #if NETSTANDARD2_0 @@ -727,10 +729,55 @@ internal static void TryKill(Process process, Action kill) } #if NETSTANDARD2_0 || NETSTANDARD2_1 - // Process.Kill(bool) requires .NET Core 3.0 or later, so the older targets can only terminate - // the process itself and not any grandchildren it spawned. - private static void Kill(Process process) => process.Kill(); + private static void Kill(Process process) => KillEntireTreeWhereSupported(process); #else private static void Kill(Process process) => process.Kill(entireProcessTree: true); #endif + + /// + /// 's entireProcessTree overload, looked up at run time. + /// + private static readonly MethodInfo? KillEntireProcessTree = typeof(Process).GetMethod(nameof(Process.Kill), [typeof(bool)]); + + /// + /// Kills and its descendants when the runtime can, and the process alone when it can't. + /// + /// The process to kill. + /// + /// The netstandard2.0/2.1 builds are what .NET Core 3.x and .NET 5–7 apps load, because no build targets those + /// runtimes. Process.Kill(bool) exists on all of them, but the netstandard reference surface does not + /// declare it, so it is found by reflection: without it a cancelled command's grandchildren kept running there + /// (ktsu-dev/RunCommand#134). .NET Framework and Mono have no such overload and kill the process alone. + /// The overload's own exceptions are rethrown unwrapped, so + /// still treats them as best effort. Compiled on every target so the tests can reach it. + /// + internal static void KillEntireTreeWhereSupported(Process process) => + KillEntireTreeWhereSupported(process, KillEntireProcessTree); + + /// + /// Kills through , or alone when it is + /// . + /// + /// The process to kill. + /// + /// Process.Kill(bool), or where the runtime lacks it. A parameter so tests can + /// stand in for .NET Framework and Mono. + /// + internal static void KillEntireTreeWhereSupported(Process process, MethodInfo? killEntireProcessTree) + { + if (killEntireProcessTree is null) + { + process.Kill(); + return; + } + + try + { + _ = killEntireProcessTree.Invoke(process, [true]); + } + catch (TargetInvocationException exception) when (exception.InnerException is not null) + { + ExceptionDispatchInfo.Capture(exception.InnerException).Throw(); + } + } }