From cc54b33c7b685a80df68d8bc924bd2e9107c05b7 Mon Sep 17 00:00:00 2001 From: borland Date: Mon, 23 Dec 2024 15:17:54 +1300 Subject: [PATCH 1/3] Change default behaviour not to swallow Cancellation exception --- source/Shellfish/ShellCommand.cs | 25 +++++++-- source/Shellfish/ShellCommandOptions.cs | 19 +++++++ ...lyExposesWhatWeWantItToExpose.approved.txt | 4 ++ source/Tests/ShellCommandFixture.StdInput.cs | 54 ++++++++++++++++++ source/Tests/ShellCommandFixture.cs | 56 ++++++++++++++++++- 5 files changed, 152 insertions(+), 6 deletions(-) create mode 100644 source/Shellfish/ShellCommandOptions.cs diff --git a/source/Shellfish/ShellCommand.cs b/source/Shellfish/ShellCommand.cs index 891d75f..c223235 100644 --- a/source/Shellfish/ShellCommand.cs +++ b/source/Shellfish/ShellCommand.cs @@ -23,6 +23,8 @@ public class ShellCommand IReadOnlyDictionary? environmentVariables; NetworkCredential? windowsCredential; Encoding? outputEncoding; + ShellCommandOptions commandOptions; + Action? onCaptureProcessId; List? stdOutTargets; List? stdErrTargets; @@ -34,6 +36,13 @@ public ShellCommand(string executable) this.executable = executable; } + // internal only, so tests can assert if a process has exited or not. + internal ShellCommand CaptureProcessId(Action onProcessId) + { + onCaptureProcessId = onProcessId; + return this; + } + /// /// Allows you to specify the working directory for the process. /// If you don't specify one, the process' Current Directory is used. @@ -146,6 +155,12 @@ public ShellCommand WithStdErrTarget(IOutputTarget target) return this; } + public ShellCommand WithOptions(ShellCommandOptions options) + { + commandOptions = options; + return this; + } + /// /// Launches the process and synchronously waits for it to exit. /// @@ -157,6 +172,8 @@ public ShellCommandResult Execute(CancellationToken cancellationToken = default) var exitedEvent = AttachProcessExitedManualResetEvent(process, cancellationToken); process.Start(); + onCaptureProcessId?.Invoke(process.Id); + IDisposable? closeStdInDisposable = BeginIoStreams(process, shouldBeginOutputRead, shouldBeginErrorRead, shouldBeginInput); try @@ -186,8 +203,7 @@ public ShellCommandResult Execute(CancellationToken cancellationToken = default) // We keep that default as it is the safest option for compatibility TryKillProcessAndChildrenRecursively(process); - // Do not rethrow; The legacy ShellExecutor didn't throw an OperationCanceledException if CancellationToken was signaled. - // This is a bit nonstandard for .NET, but we keep it as the default for compatibility. + if (commandOptions != ShellCommandOptions.DoNotThrowOnCancellation) throw; } finally { @@ -207,6 +223,8 @@ public async Task ExecuteAsync(CancellationToken cancellatio var exitedTask = AttachProcessExitedTask(process, cancellationToken); process.Start(); + + onCaptureProcessId?.Invoke(process.Id); IDisposable? closeStdInDisposable = BeginIoStreams(process, shouldBeginOutputRead, shouldBeginErrorRead, shouldBeginInput); @@ -232,8 +250,7 @@ public async Task ExecuteAsync(CancellationToken cancellatio // We keep that default as it is the safest option for compatibility TryKillProcessAndChildrenRecursively(process); - // Do not rethrow; The legacy ShellExecutor didn't throw an OperationCanceledException if CancellationToken was signaled. - // This is a bit nonstandard for .NET, but we keep it as the default for compatibility. + if (commandOptions != ShellCommandOptions.DoNotThrowOnCancellation) throw; } finally { diff --git a/source/Shellfish/ShellCommandOptions.cs b/source/Shellfish/ShellCommandOptions.cs new file mode 100644 index 0000000..57a90d9 --- /dev/null +++ b/source/Shellfish/ShellCommandOptions.cs @@ -0,0 +1,19 @@ +using System; + +namespace Octopus.Shellfish; + +public enum ShellCommandOptions +{ + /// + /// Default value, equivalent to not specifying any options. + /// + None = 0, + + /// + /// By default, if the CancellationToken is cancelled, the running process will be killed, and an OperationCanceledException + /// will be thrown, like the vast majority of other .NET code. + /// However, the legacy ShellExecutor API would swallow OperationCanceledException exceptions on cancellation, so this + /// option exists to preserve that behaviour where necessary. + /// + DoNotThrowOnCancellation, +} \ No newline at end of file diff --git a/source/Tests/PublicSurfaceArea.TheLibraryOnlyExposesWhatWeWantItToExpose.approved.txt b/source/Tests/PublicSurfaceArea.TheLibraryOnlyExposesWhatWeWantItToExpose.approved.txt index b755a78..64a994a 100644 --- a/source/Tests/PublicSurfaceArea.TheLibraryOnlyExposesWhatWeWantItToExpose.approved.txt +++ b/source/Tests/PublicSurfaceArea.TheLibraryOnlyExposesWhatWeWantItToExpose.approved.txt @@ -12,8 +12,12 @@ Octopus.Shellfish.ShellCommand.WithCredentials Octopus.Shellfish.ShellCommand.WithOutputEncoding Octopus.Shellfish.ShellCommand.WithStdOutTarget Octopus.Shellfish.ShellCommand.WithStdErrTarget +Octopus.Shellfish.ShellCommand.WithOptions Octopus.Shellfish.ShellCommand.Execute Octopus.Shellfish.ShellCommand.ExecuteAsync +Octopus.Shellfish.ShellCommandOptions.value__ +Octopus.Shellfish.ShellCommandOptions.None +Octopus.Shellfish.ShellCommandOptions.DoNotThrowOnCancellation Octopus.Shellfish.ShellCommandResult.ExitCode Octopus.Shellfish.ShellExecutionException.Errors Octopus.Shellfish.ShellExecutionException.Message diff --git a/source/Tests/ShellCommandFixture.StdInput.cs b/source/Tests/ShellCommandFixture.StdInput.cs index a8ba8cf..be2d371 100644 --- a/source/Tests/ShellCommandFixture.StdInput.cs +++ b/source/Tests/ShellCommandFixture.StdInput.cs @@ -225,6 +225,60 @@ read name }) .WithStdErrTarget(stdErr); + var cancellationToken = cts.Token; + if (behaviour == SyncBehaviour.Async) + { + await executor.Invoking(e => e.ExecuteAsync(cancellationToken)).Should().ThrowAsync(); + } + else + { + executor.Invoking(e => e.Execute(cancellationToken)).Should().Throw(); + } + + // we can't observe any exit code because Execute() threw an exception + stdErr.ToString().Should().BeEmpty("no messages should be written to stderr"); + stdOut.ToString().Should().BeOneOf([ + "Enter Name:" + Environment.NewLine, + "Enter Name:" + Environment.NewLine + "Hello ''" + Environment.NewLine, + ], because: "When we cancel the process we close StdIn and it shuts down. The process observes the EOF as empty string and prints 'Hello ' but there is a benign race condition which means we may not observe this output. Test needs to handle both cases"); + } + + [Theory, InlineData(SyncBehaviour.Sync), InlineData(SyncBehaviour.Async)] + public async Task ShouldBeCancellable_DoNotThrowOnCancellation(SyncBehaviour behaviour) + { + using var tempScript = TempScript.Create( + cmd: """ + @echo off + echo Enter Name: + set /p name= + echo Hello '%name%' + """, + sh: """ + echo "Enter Name:" + read name + echo "Hello '$name'" + """); + + var stdOut = new StringBuilder(); + var stdErr = new StringBuilder(); + + // it's going to ask us for the name first, but we don't give it anything; the script should hang + var stdIn = new TestInputSource(); + + using var cts = CancellationTokenSource.CreateLinkedTokenSource(CancellationToken); + + var executor = new ShellCommand(tempScript.GetHostExecutable()) + .WithOptions(ShellCommandOptions.DoNotThrowOnCancellation) + .WithArguments(tempScript.GetCommandArgs()) + .WithStdInSource(stdIn) + .WithStdOutTarget(stdOut) + .WithStdOutTarget(l => + { + // when we receive the first prompt, cancel and kill the process + if (l.Contains("Enter Name:")) cts.Cancel(); + }) + .WithStdErrTarget(stdErr); + var result = behaviour == SyncBehaviour.Async ? await executor.ExecuteAsync(cts.Token) : executor.Execute(cts.Token); diff --git a/source/Tests/ShellCommandFixture.cs b/source/Tests/ShellCommandFixture.cs index da9e276..82fdb58 100644 --- a/source/Tests/ShellCommandFixture.cs +++ b/source/Tests/ShellCommandFixture.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Runtime.InteropServices; using System.Text; using System.Threading; @@ -86,10 +87,58 @@ public async Task CancellationToken_ShouldForceKillTheProcess(SyncBehaviour beha // Terminate the process after a very short time so the test doesn't run forever cts.CancelAfter(TimeSpan.FromSeconds(1)); + var executor = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) + ? new ShellCommand("cmd.exe").WithArguments("timeout /t 500 /nobreak") + : new ShellCommand("bash").WithArguments("-c \"sleep 500\""); + + int processId = 0; var stdOut = new StringBuilder(); var stdErr = new StringBuilder(); - // Starting a new instance of cmd.exe will run indefinitely waiting for user input - var executor = new ShellCommand(Command) + executor = executor + .CaptureProcessId(pid => processId = pid) + .WithStdOutTarget(stdOut) + .WithStdErrTarget(stdErr); + + var cancellationToken = cts.Token; + if (behaviour == SyncBehaviour.Async) + { + await executor.Invoking(e => e.ExecuteAsync(cancellationToken)).Should().ThrowAsync(); + } + else + { + executor.Invoking(e => e.Execute(cancellationToken)).Should().Throw(); + } + // we can't observe any exit code because Execute() threw an exception + + if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + { + stdOut.ToString().Should().ContainEquivalentOf("Microsoft Windows", "the default command-line header would be written to stdout"); + } + + stdErr.ToString().Should().BeEmpty("no messages should be written to stderr, and the process was terminated before the trailing newline got there"); + + processId.Should().NotBe(0, "the process ID should have been captured"); + new Action(() => Process.GetProcessById(processId)).Should().Throw().Which.Message.Should().Contain("not running"); + } + + [Theory, InlineData(SyncBehaviour.Sync), InlineData(SyncBehaviour.Async)] + public async Task CancellationToken_ShouldForceKillTheProcess_DoNotThrowOnCancellation(SyncBehaviour behaviour) + { + using var cts = CancellationTokenSource.CreateLinkedTokenSource(CancellationToken); + // Terminate the process after a very short time so the test doesn't run forever + cts.CancelAfter(TimeSpan.FromSeconds(1)); + + int processId = 0; + var stdOut = new StringBuilder(); + var stdErr = new StringBuilder(); + + var executor = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) + ? new ShellCommand("cmd.exe").WithArguments("timeout /t 500 /nobreak") + : new ShellCommand("bash").WithArguments("-c \"sleep 500\""); + + executor = executor + .WithOptions(ShellCommandOptions.DoNotThrowOnCancellation) + .CaptureProcessId(pid => processId = pid) .WithStdOutTarget(stdOut) .WithStdErrTarget(stdErr); @@ -110,6 +159,9 @@ public async Task CancellationToken_ShouldForceKillTheProcess(SyncBehaviour beha } stdErr.ToString().Should().BeEmpty("no messages should be written to stderr, and the process was terminated before the trailing newline got there"); + + processId.Should().NotBe(0, "the process ID should have been captured"); + new Action(() => Process.GetProcessById(processId)).Should().Throw().Which.Message.Should().Contain("not running"); } [Theory, InlineData(SyncBehaviour.Sync), InlineData(SyncBehaviour.Async)] From a29401feb5b1fbd636b3e2f5e0cda76bb28e53aa Mon Sep 17 00:00:00 2001 From: borland Date: Tue, 21 Jan 2025 07:16:03 +1300 Subject: [PATCH 2/3] Fix tests; windows timeout.exe is finicky --- source/Tests/ShellCommandFixture.cs | 38 +++++++++-------------------- 1 file changed, 12 insertions(+), 26 deletions(-) diff --git a/source/Tests/ShellCommandFixture.cs b/source/Tests/ShellCommandFixture.cs index 82fdb58..ab67966 100644 --- a/source/Tests/ShellCommandFixture.cs +++ b/source/Tests/ShellCommandFixture.cs @@ -84,20 +84,18 @@ public async Task RunningAsSameUser_ShouldCopySpecialEnvironmentVariables(SyncBe public async Task CancellationToken_ShouldForceKillTheProcess(SyncBehaviour behaviour) { using var cts = CancellationTokenSource.CreateLinkedTokenSource(CancellationToken); - // Terminate the process after a very short time so the test doesn't run forever - cts.CancelAfter(TimeSpan.FromSeconds(1)); + // Terminate the process after a short time so the test doesn't run forever + cts.CancelAfter(TimeSpan.FromSeconds(0.5)); var executor = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) - ? new ShellCommand("cmd.exe").WithArguments("timeout /t 500 /nobreak") + ? new ShellCommand("timeout.exe").WithArguments("/t 500 /nobreak") : new ShellCommand("bash").WithArguments("-c \"sleep 500\""); int processId = 0; - var stdOut = new StringBuilder(); - var stdErr = new StringBuilder(); + executor = executor - .CaptureProcessId(pid => processId = pid) - .WithStdOutTarget(stdOut) - .WithStdErrTarget(stdErr); + .CaptureProcessId(pid => processId = pid); + // Do not capture stdout or stderr; the windows timeout command will fail with ERROR: Input redirection is not supported var cancellationToken = cts.Token; if (behaviour == SyncBehaviour.Async) @@ -108,15 +106,9 @@ public async Task CancellationToken_ShouldForceKillTheProcess(SyncBehaviour beha { executor.Invoking(e => e.Execute(cancellationToken)).Should().Throw(); } + // we can't observe any exit code because Execute() threw an exception - if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) - { - stdOut.ToString().Should().ContainEquivalentOf("Microsoft Windows", "the default command-line header would be written to stdout"); - } - - stdErr.ToString().Should().BeEmpty("no messages should be written to stderr, and the process was terminated before the trailing newline got there"); - processId.Should().NotBe(0, "the process ID should have been captured"); new Action(() => Process.GetProcessById(processId)).Should().Throw().Which.Message.Should().Contain("not running"); } @@ -125,22 +117,19 @@ public async Task CancellationToken_ShouldForceKillTheProcess(SyncBehaviour beha public async Task CancellationToken_ShouldForceKillTheProcess_DoNotThrowOnCancellation(SyncBehaviour behaviour) { using var cts = CancellationTokenSource.CreateLinkedTokenSource(CancellationToken); - // Terminate the process after a very short time so the test doesn't run forever - cts.CancelAfter(TimeSpan.FromSeconds(1)); + // Terminate the process after a short time so the test doesn't run forever + cts.CancelAfter(TimeSpan.FromSeconds(0.5)); int processId = 0; - var stdOut = new StringBuilder(); - var stdErr = new StringBuilder(); var executor = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) - ? new ShellCommand("cmd.exe").WithArguments("timeout /t 500 /nobreak") + ? new ShellCommand("timeout.exe").WithArguments("/t 500 /nobreak") : new ShellCommand("bash").WithArguments("-c \"sleep 500\""); executor = executor .WithOptions(ShellCommandOptions.DoNotThrowOnCancellation) - .CaptureProcessId(pid => processId = pid) - .WithStdOutTarget(stdOut) - .WithStdErrTarget(stdErr); + .CaptureProcessId(pid => processId = pid); + // Do not capture stdout or stderr; the windows timeout command will fail with ERROR: Input redirection is not supported var result = behaviour == SyncBehaviour.Async ? await executor.ExecuteAsync(cts.Token) @@ -151,15 +140,12 @@ public async Task CancellationToken_ShouldForceKillTheProcess_DoNotThrowOnCancel if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) { exitCode.Should().BeLessOrEqualTo(0, "the process should have been terminated"); - stdOut.ToString().Should().ContainEquivalentOf("Microsoft Windows", "the default command-line header would be written to stdout"); } else { exitCode.Should().BeOneOf(SIG_KILL, SIG_TERM, 0, -1); } - stdErr.ToString().Should().BeEmpty("no messages should be written to stderr, and the process was terminated before the trailing newline got there"); - processId.Should().NotBe(0, "the process ID should have been captured"); new Action(() => Process.GetProcessById(processId)).Should().Throw().Which.Message.Should().Contain("not running"); } From 4521679eba1abe96e3dc17a2361e53b5feb411de Mon Sep 17 00:00:00 2001 From: borland Date: Tue, 21 Jan 2025 07:42:57 +1300 Subject: [PATCH 3/3] Asserting based on ProcessId's isn't reliable as OS'es frequently recycle them for other processes --- source/Shellfish/ShellCommand.cs | 10 +++++----- source/Tests/ShellCommandFixture.cs | 30 +++++++++++++++++++---------- 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/source/Shellfish/ShellCommand.cs b/source/Shellfish/ShellCommand.cs index c223235..a2e0f04 100644 --- a/source/Shellfish/ShellCommand.cs +++ b/source/Shellfish/ShellCommand.cs @@ -24,7 +24,7 @@ public class ShellCommand NetworkCredential? windowsCredential; Encoding? outputEncoding; ShellCommandOptions commandOptions; - Action? onCaptureProcessId; + Action? onCaptureProcess; List? stdOutTargets; List? stdErrTargets; @@ -37,9 +37,9 @@ public ShellCommand(string executable) } // internal only, so tests can assert if a process has exited or not. - internal ShellCommand CaptureProcessId(Action onProcessId) + internal ShellCommand CaptureProcess(Action onProcess) { - onCaptureProcessId = onProcessId; + onCaptureProcess = onProcess; return this; } @@ -172,7 +172,7 @@ public ShellCommandResult Execute(CancellationToken cancellationToken = default) var exitedEvent = AttachProcessExitedManualResetEvent(process, cancellationToken); process.Start(); - onCaptureProcessId?.Invoke(process.Id); + onCaptureProcess?.Invoke(process); IDisposable? closeStdInDisposable = BeginIoStreams(process, shouldBeginOutputRead, shouldBeginErrorRead, shouldBeginInput); @@ -224,7 +224,7 @@ public async Task ExecuteAsync(CancellationToken cancellatio var exitedTask = AttachProcessExitedTask(process, cancellationToken); process.Start(); - onCaptureProcessId?.Invoke(process.Id); + onCaptureProcess?.Invoke(process); IDisposable? closeStdInDisposable = BeginIoStreams(process, shouldBeginOutputRead, shouldBeginErrorRead, shouldBeginInput); diff --git a/source/Tests/ShellCommandFixture.cs b/source/Tests/ShellCommandFixture.cs index ab67966..24ee9a5 100644 --- a/source/Tests/ShellCommandFixture.cs +++ b/source/Tests/ShellCommandFixture.cs @@ -91,10 +91,9 @@ public async Task CancellationToken_ShouldForceKillTheProcess(SyncBehaviour beha ? new ShellCommand("timeout.exe").WithArguments("/t 500 /nobreak") : new ShellCommand("bash").WithArguments("-c \"sleep 500\""); - int processId = 0; - + Process? process = null; executor = executor - .CaptureProcessId(pid => processId = pid); + .CaptureProcess(p => process = p); // Do not capture stdout or stderr; the windows timeout command will fail with ERROR: Input redirection is not supported var cancellationToken = cts.Token; @@ -109,8 +108,8 @@ public async Task CancellationToken_ShouldForceKillTheProcess(SyncBehaviour beha // we can't observe any exit code because Execute() threw an exception - processId.Should().NotBe(0, "the process ID should have been captured"); - new Action(() => Process.GetProcessById(processId)).Should().Throw().Which.Message.Should().Contain("not running"); + process?.Should().NotBeNull(); + EnsureProcessHasExited(process!); } [Theory, InlineData(SyncBehaviour.Sync), InlineData(SyncBehaviour.Async)] @@ -119,16 +118,15 @@ public async Task CancellationToken_ShouldForceKillTheProcess_DoNotThrowOnCancel using var cts = CancellationTokenSource.CreateLinkedTokenSource(CancellationToken); // Terminate the process after a short time so the test doesn't run forever cts.CancelAfter(TimeSpan.FromSeconds(0.5)); - - int processId = 0; var executor = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? new ShellCommand("timeout.exe").WithArguments("/t 500 /nobreak") : new ShellCommand("bash").WithArguments("-c \"sleep 500\""); + Process? process = null; executor = executor .WithOptions(ShellCommandOptions.DoNotThrowOnCancellation) - .CaptureProcessId(pid => processId = pid); + .CaptureProcess(p => process = p); // Do not capture stdout or stderr; the windows timeout command will fail with ERROR: Input redirection is not supported var result = behaviour == SyncBehaviour.Async @@ -146,8 +144,20 @@ public async Task CancellationToken_ShouldForceKillTheProcess_DoNotThrowOnCancel exitCode.Should().BeOneOf(SIG_KILL, SIG_TERM, 0, -1); } - processId.Should().NotBe(0, "the process ID should have been captured"); - new Action(() => Process.GetProcessById(processId)).Should().Throw().Which.Message.Should().Contain("not running"); + process?.Should().NotBeNull(); + EnsureProcessHasExited(process!); + } + + static void EnsureProcessHasExited(Process process) + { + try + { + process.HasExited.Should().BeTrue("the process should have exited"); + } + catch (InvalidOperationException e) when (e.Message is "No process is associated with this object.") + { + // process.HasExited throws this exception if you call HasExited on a process that has quit already; we expect this + } } [Theory, InlineData(SyncBehaviour.Sync), InlineData(SyncBehaviour.Async)]