Change ShellCommand default behaviour not to swallow Cancellation exception

[borland]

Background

When the new ShellCommand API was added, we chose to preserve the cancellation behaviour of the old ShellExecutor API.

The legacy behaviour is such:

• You call Execute(cancellationToken);
• The token gets cancelled before the process exits;
• Shellfish would terminate the launched process ;
• However it would NOT throw an OperationCanceledException, it would return successfully. This contradicts the behaviour of essentially all other cancellable functions in .NET.

This "preservation" behaviour was chosen to make migrating other code to the new API easier, and seemed sensible at the time.

However, we have since learnt:

• The cost of the weird legacy API was higher than expected; it has confused people on multiple occasions when they've reviewed or tried to use the new Shellfish
• The primary reason we wanted to preserve the old behaviour was because we thought Task execution in Octopus Server used this exception-swallowing behaviour, and it's an area we have to be extremely careful with if/when we migrate it to the new Shellfish API. However it was re-discovered that since November 2023, Octopus Server doesn't expect exception-swallowing anyway

Results

• Adds a new enum ShellCommandOptions with values None and DoNotThrowOnCancellation

• Adds a new WithOptions function to ShellCommand, so you can do command.WithOptions(ShellCommandOptions.DoNotThrowOnCancellation);

• Changes the default behaviour so it rethrows the underlying OperationCanceledException as per other .NET code.

• Updates tests accordingly

Note There were two extra test changes here

• A CaptureProcess method was added to ShellCommand. This is to allow tests to get the System.Diagnostics.Process object and assert that a process has actually exited. Deliberately non-public as we don't want to commit to that level of information in the public API yet.

• Tests that wanted to block used to run cmd or bash with no arguments, expecting them to wait forever for stdinput. We recently learnt that in linux on TeamCity, bash is capable of determining that it has no stdinput and is not running interactively and exits immediately, which would invalidate the test. I switched to using "sleep" on linux and "timeout.exe" on windows instead of relying on stdinput.

[borland]

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.

[YuKitsune]

What's with this value__?

[borland]

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 🤷

[adam-mccoy]

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.

[borland]

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)