Moving CancelOnProcessTermination out of middleware, better cancellation support

[adamsitnik]

Asynchronous APIs have two advantages: improved scalability and cancellation support. In the case of command line applications, we mostly care about the latter.

In my opinion storing the CancellationToken inside InvocationContext is not intuitive, as C# devs are just used to passing an instance of CT to async methods. I expect that most of the S.CL users use InvokeAsync and never check for the cancellation, so they don’t take advantage of any async benefits.

I’ve made CancellationToken a mandatory argument for the async SetHandler overload. Now, when the users don’t pass it to any async method used inside the handler, they are going to get a compiler warning:

command.SetHandler(async (context, cancellationToken) =>
{
    await Task.Delay(Timeout.InfiniteTimeSpan);
});

Forward the 'cancellationToken' parameter to the 'Delay' method or pass in 'CancellationToken.None' explicitly to indicate intentionally not propagating the token

It allows us for removing GetCancellationToken, LinkToken and Dispose from InvocationContext and gets us closer to the goal of replacing all usages of InvocationContext with just ParseResult.

Another goal was moving CancelOnProcessTermination out of middleware. This was not easy, but I've managed to get it working and make it easier to use.

What I don't like is adding another argument to GetHandler extension methods. Since we are going to remove all of them soon, I hope it's not an issue.

The app I've used for testing:

// Copyright (c) .NET Foundation and contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Threading.Tasks;
using System.Threading;
using System.CommandLine.Parsing;

namespace System.CommandLine.Benchmarks
{
    class Program
    {
        static async Task<int> Main(string[] args)
        {
            Argument<bool> argument = new()
            {
                Arity = ArgumentArity.ExactlyOne
            };

            RootCommand command = new ()
            {
                argument
            };

            command.SetHandler(async (context, cancellationToken) =>
            {
                try
                {
                    bool value = context.GetValue(argument);
                    string tokenDesc = value ? "the right token" : "the wrong token";
                    context.Console.WriteLine($"Starting infinite delay using {tokenDesc}");

                    CancellationToken token = value 
                        ? cancellationToken // the cancellation will work fine
                        : CancellationToken.None; // the timeout passed to CancelOnProcessTermination will be used

                    await Task.Delay(Timeout.InfiniteTimeSpan, token);
                    context.ExitCode = 1;
                }
                catch (OperationCanceledException)
                {
                    context.Console.WriteLine("Cancellation worked as expected");
                }
            });

            var built = new CommandLineBuilder(command)
               .CancelOnProcessTermination(TimeSpan.FromSeconds(1))
               .UseExceptionHandler()
               .Build();

            int exitCode = await built.InvokeAsync(args);
            Console.WriteLine($"Exit code={exitCode}");
            return exitCode;
        }
    }
}

fixes #1783
fixes #1921
fixes #2019

[adamsitnik]

@jonsequitur the Linux CI leg might be red, but please take a look at the PR description and let me know what do you think about such changes. I want to make sure I am going in the right direction before I spend more time working on it.

[jonsequitur]

There's a third reason why people use async methods and based on feedback from people using this API, this one is much more common than the two mentioned: the need to call async APIs. Given this, I think requiring the parameter might be adding some ceremony where we're hoping to reduce it.

[jonsequitur]

Any redesign of cancelation probably deserves its own discussion as there are a few different use cases to consider.

Related: #1783, #1921

[Keboo]

Any reason for using the runtime platform check rather than making a LinuxOnlyFact?

[adamsitnik]

My initial intention was to enable it on macOS too, but then it turned out that RemoteExecutor does not support this scenario, so instead of adding new attribute (This repo defines only LinuxOnlyTheory attribute. ) I've added the ugly if statement.

[adamsitnik]

@Keboo thanks a lot for the review!

[KalleOlaviNiemitalo]

This PR had an unexpected effect in my application.

I had code like the following:

public static RootCommand MakeRootCommand()
{
    var rootCommand = new RootCommand();
    AddSymbols(rootCommand);
    rootCommand.SetHandler(
        (InvocationContext context) => MyHandler.InvokeAsync(
            parameters: GetMyCommandParameters(context),
            console: context.Console));
    return rootCommand;
}

private static async Task<int> InvokeAsync(
    MyCommandParameters parameters,
    System.CommandLine.IConsole console)
{
    …
}

That code used to call SetHandler(this Command, Func<InvocationContext, Task>), but after this PR, it started calling SetHandler(this Command, Action<InvocationContext>) instead; thus discarding the Task that InvokeAsync returned. Fortunately, not awaiting the Task caused warning CS4014. Anyway, this is a risk for developers upgrading from prior versions of System.CommandLine.

[adamsitnik]

Anyway, this is a risk for developers upgrading from prior versions of System.CommandLine.

Thanks for the feedback! I currently don't see a way to avoid that, but in the very near future we are most likely going to replace InvocationContext accepting handlers with ParseResult based ones. I hope that this will force the users to switch to the right overload.

And in general the SetHandler is still far for perfect, we are opened to suggestions in improving it.