diff --git a/.gitignore b/.gitignore index dc0470a..e043c9f 100644 --- a/.gitignore +++ b/.gitignore @@ -203,6 +203,11 @@ PublishScripts/ **/[Pp]ackages/* # except build/, which is used as an MSBuild target. !**/[Pp]ackages/build/ +# and except a Unity project's Packages/, which is source: Unity's package manifest and its +# resolved lock file are both meant to be committed, and a NuGet restore folder never contains +# a file by either name. +!**/[Pp]ackages/manifest.json +!**/[Pp]ackages/packages-lock.json # Uncomment if necessary however generally it will be regenerated when needed #!**/[Pp]ackages/repositories.config # NuGet v3's project.json files produces more ignorable files @@ -651,3 +656,16 @@ Temporary Items # ImGui.ini files imgui.ini + +# Game engine projects +# +# Godot: the import cache, and the mono/temp bin+obj a C# build writes. +.godot/ + +# Unity: .meta files are source, not the Visual Studio C++ build artifact that the `*.meta` rule +# further up targets. Unity generates one per asset and it carries the GUID that scenes, prefabs +# and serialized references point at, so ignoring them gives every clone fresh GUIDs and silently +# breaks those references - including for a plug-in whose .dll is itself a build output. This +# negation has to come after that rule to win, and is scoped to the asset tree so the Visual +# Studio artifact stays ignored everywhere else. +!**/[Aa]ssets/**/*.meta diff --git a/RunCommand.Test/RunCommandTests.cs b/RunCommand.Test/RunCommandTests.cs index 9431241..767cfbc 100644 --- a/RunCommand.Test/RunCommandTests.cs +++ b/RunCommand.Test/RunCommandTests.cs @@ -450,6 +450,42 @@ public async Task ExecuteAsyncShouldTerminateProcessWhenCancelledWhileRunning() await Assert.ThrowsAsync(() => execution).ConfigureAwait(false); } + [TestMethod] + public void CancelShouldNotThrowWhenTheTreeKillCannotTerminateEveryDescendant() + { + (string fileName, string[] arguments) = GetSleepCommand(); + using Process process = Process.Start(new ProcessStartInfo(fileName, arguments) { UseShellExecute = false })!; + + try + { + // What Process.Kill(entireProcessTree: true) throws when a descendant belongs to another + // user or runs elevated. + static void PartlyFailingKill(Process _) => + throw new AggregateException( + "Not all processes in the process tree could be terminated.", + new System.ComponentModel.Win32Exception(1, "Operation not permitted")); + + // Registered the way RunAsync registers its kill, so this is the caller's Cancel(). + using CancellationTokenSource cancellationTokenSource = new(); + using CancellationTokenRegistration registration = cancellationTokenSource.Token.Register( + () => RunCommand.TryKill(process, PartlyFailingKill)); + + cancellationTokenSource.Cancel(); + Assert.IsTrue(cancellationTokenSource.IsCancellationRequested, "Expected Cancel() to return normally after the kill failed."); + + // Called directly as well, as the catch blocks in RunAsync do: an exception here would + // replace the OperationCanceledException or the handler failure being reported. + RunCommand.TryKill(process, PartlyFailingKill); + + // Only the injected kill ran, so the process is still alive for the finally block to end. + Assert.IsFalse(process.HasExited, "Expected only the failing kill to have run."); + } + finally + { + process.Kill(entireProcessTree: true); + } + } + [TestMethod] public async Task CancelShouldNotRunTheCallersContinuationInline() { diff --git a/RunCommand/RunCommand.cs b/RunCommand/RunCommand.cs index 8ed9571..9d41adb 100644 --- a/RunCommand/RunCommand.cs +++ b/RunCommand/RunCommand.cs @@ -533,19 +533,26 @@ private static async Task RunAsync(ProcessStartInfo startInfo, OutputHandle return process.ExitCode; } - private static void TryKill(Process process) + private static void TryKill(Process process) => TryKill(process, Kill); + + /// + /// Kills through , treating every way the kill + /// can fail as best effort. + /// + /// + /// This runs inside the cancellation token's registration, so anything it throws comes out of + /// the caller's , and it runs in catch blocks, + /// where anything it throws replaces the exception being reported. It must never throw. + /// + /// The process to kill. + /// Performs the kill. A parameter so tests can make it fail. + internal static void TryKill(Process process, Action kill) { try { if (!process.HasExited) { -#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. - process.Kill(); -#else - process.Kill(entireProcessTree: true); -#endif + kill(process); } } catch (InvalidOperationException) @@ -560,5 +567,19 @@ private static void TryKill(Process process) { // Terminating a remote process is not supported. } + catch (AggregateException) + { + // Kill(entireProcessTree) reports descendants it could not terminate, such as ones owned + // by another user or running elevated, this way. It carries on past each failure, so + // everything it could kill has already been killed. + } } + +#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(); +#else + private static void Kill(Process process) => process.Kill(entireProcessTree: true); +#endif }