Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change ShellCommand default behaviour not to swallow Cancellation exception #134

Merged
merged 3 commits into from
Jan 20, 2025
Merged

Change ShellCommand default behaviour not to swallow Cancellation exception #134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc54b33
Change default behaviour not to swallow Cancellation exception
borland Dec 23, 2024
a29401f
Fix tests; windows timeout.exe is finicky
borland Jan 20, 2025
4521679
Asserting based on ProcessId's isn't reliable as OS'es frequently rec…
borland Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions source/Shellfish/ShellCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ public class ShellCommand
IReadOnlyDictionary<string, string>? environmentVariables;
NetworkCredential? windowsCredential;
Encoding? outputEncoding;
ShellCommandOptions commandOptions;
Action<Process>? onCaptureProcess;

List<IOutputTarget>? stdOutTargets;
List<IOutputTarget>? stdErrTargets;
Expand All @@ -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 CaptureProcess(Action<Process> onProcess)
{
onCaptureProcess = onProcess;
return this;
}

/// <summary>
/// Allows you to specify the working directory for the process.
/// If you don't specify one, the process' Current Directory is used.
Expand Down Expand Up @@ -146,6 +155,12 @@ public ShellCommand WithStdErrTarget(IOutputTarget target)
return this;
}

public ShellCommand WithOptions(ShellCommandOptions options)
{
commandOptions = options;
return this;
}

/// <summary>
/// Launches the process and synchronously waits for it to exit.
/// </summary>
Expand All @@ -157,6 +172,8 @@ public ShellCommandResult Execute(CancellationToken cancellationToken = default)
var exitedEvent = AttachProcessExitedManualResetEvent(process, cancellationToken);
process.Start();

onCaptureProcess?.Invoke(process);

IDisposable? closeStdInDisposable = BeginIoStreams(process, shouldBeginOutputRead, shouldBeginErrorRead, shouldBeginInput);

try
Expand Down Expand Up @@ -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
{
Expand All @@ -207,6 +223,8 @@ public async Task<ShellCommandResult> ExecuteAsync(CancellationToken cancellatio

var exitedTask = AttachProcessExitedTask(process, cancellationToken);
process.Start();

onCaptureProcess?.Invoke(process);

IDisposable? closeStdInDisposable = BeginIoStreams(process, shouldBeginOutputRead, shouldBeginErrorRead, shouldBeginInput);

Expand All @@ -232,8 +250,7 @@ public async Task<ShellCommandResult> 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
{
Expand Down
19 changes: 19 additions & 0 deletions source/Shellfish/ShellCommandOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
using System;

namespace Octopus.Shellfish;

public enum ShellCommandOptions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using an enum means this will be a list of mutually exclusive boolean options (or not, if we added [Flags]). It's difficult to predict what options we might want to add in future, but this seems quite limited. I think a class would provide more flexibility.

Copy link
Contributor Author

@borland borland Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but it's hard to predict the future. We might only have this one option for the next 7 years, in which case classes would be additional trouble for no benefit.

Given the limited use of Shellfish, if we find a future scenario where we need more kinds of options, I'd happily commit to a breaking API change at that point. (Note also: making it [Flags] is nonbreaking because we only have 0 and 1 right now)

{
/// <summary>
/// Default value, equivalent to not specifying any options.
/// </summary>
None = 0,

/// <summary>
/// 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.
/// </summary>
DoNotThrowOnCancellation,
}
4 changes: 4 additions & 0 deletions source/Tests/PublicSurfaceArea.TheLibraryOnlyExposesWhatWeWantItToExpose.approved.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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__
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's with this value__?

Copy link
Contributor Author

@borland borland Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's how enums work in C#, if you decompile it in sharplab you get this

.class public auto ansi sealed ShellCommandOptions
    extends [System.Runtime]System.Enum
{
    // Fields
    .field public specialname rtspecialname int32 value__
    .field public static literal valuetype ShellCommandOptions None = int32(0)
    .field public static literal valuetype ShellCommandOptions DoNotThrowOnCancellation = int32(1)

} // end of class ShellCommandOptions

The reflection-scanny thing is obviously picking it up. It's just a convention test so 🤷

Octopus.Shellfish.ShellCommandOptions.None
Octopus.Shellfish.ShellCommandOptions.DoNotThrowOnCancellation
Octopus.Shellfish.ShellCommandResult.ExitCode
Octopus.Shellfish.ShellExecutionException.Errors
Octopus.Shellfish.ShellExecutionException.Message
Expand Down
54 changes: 54 additions & 0 deletions source/Tests/ShellCommandFixture.StdInput.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<OperationCanceledException>();
}
else
{
executor.Invoking(e => e.Execute(cancellationToken)).Should().Throw<OperationCanceledException>();
}

// 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)]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff is a bit weird. This is the pre-existing test; it has no modifications other than adding the _DoNotThrowOnCancellation name suffix and setting .WithOptions(ShellCommandOptions.DoNotThrowOnCancellation)

The above test is the new one, it is a copy-paste with tweaks to handle the exception on cancel.

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);
Expand Down
68 changes: 58 additions & 10 deletions source/Tests/ShellCommandFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Runtime.InteropServices;
using System.Text;
using System.Threading;
Expand Down Expand Up @@ -83,15 +84,50 @@ 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 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)
.WithStdOutTarget(stdOut)
.WithStdErrTarget(stdErr);
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
.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;
if (behaviour == SyncBehaviour.Async)
{
await executor.Invoking(e => e.ExecuteAsync(cancellationToken)).Should().ThrowAsync<OperationCanceledException>();
}
else
{
executor.Invoking(e => e.Execute(cancellationToken)).Should().Throw<OperationCanceledException>();
}

// we can't observe any exit code because Execute() threw an exception

process?.Should().NotBeNull();
EnsureProcessHasExited(process!);
}

[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 short time so the test doesn't run forever
cts.CancelAfter(TimeSpan.FromSeconds(0.5));

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)
.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
? await executor.ExecuteAsync(cts.Token)
Expand All @@ -102,14 +138,26 @@ public async Task CancellationToken_ShouldForceKillTheProcess(SyncBehaviour beha
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");
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)]
Expand Down