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.

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

CLI Command RunAsync support passing in/propagating a CancelationToken #701

Open
Simonl9l opened this issue Jan 24, 2022 · 13 comments
Open
Labels
feature ⭐ top feature Top feature request. ⭐ top issue Top issue.

Comments

@Simonl9l
Copy link

Simonl9l commented Jan 24, 2022

Is your feature request related to a problem? Please describe.
By convention in DotNet Core one wild supply a CancellationToken into an Async method when awaited!

Describe the solution you'd like
Support passing in a CancelationToken such that is propagated to the AsyncCommand.ExecuteAsync and for that matter provide a ValidateAsync that also takes the token.

In this way a command handler can detect it's being canceled (via async Main for example)

Describe alternatives you've considered

One can make the organ available via static and class scoped but its not very well encapsulated.

Additional context
N/A


Please upvote 👍 this issue if you are interested in it.

@kuhnboy
Copy link

kuhnboy commented Jan 24, 2022

Why? Can't you do this:

CancellationToken token; // From Somewhere else

AnsiConsole.Progress().StartAsync(async ctx =>
    {
          doStuff(token);
    });

@nils-a
Copy link
Contributor

nils-a commented Jan 24, 2022

@kuhnboy, I think what @Simonl9l asked for here is:

  • ICommandApp.RunAsync(IEnumerable<string> args)
    • ➡️ ICommandApp.RunAsync(IEnumerable<string> args, CancellationToken cancellationToken = default(CancellationToken))
  • AsyncCommand.ExecuteAsync(CommandContext context)
    • ➡️ AsyncCommand.ExecuteAsync(CommandContext context, CancellationToken cancellationToken = default(CancellationToken))
  • AsyncCommand.ExecuteAsync<TSettings>(CommandContext context, TSettings settings)
    • ➡️ AsyncCommand.ExecuteAsync<TSettings>(CommandContext context, TSettings settings, CancellationToken cancellationToken = default(CancellationToken))
  • new:
    • AsyncCommand.ValidateAsync<TSettings>(CommandContext context, TSettings settings, CancellationToken cancellationToken = default(CancellationToken))

@Simonl9l
Copy link
Author

Hi @kuhnboy & @nils-a thanks for the replies and clarifications...I've been away apologies for the wait in my reply!

in the version of the Spector.Console library I'm using 0.43.0 there seem to be no versions of these methods that support the CancellationToken argument.

Whist I "could" DI the Token in to the AsyncCommand constructor this seems very unconventional.

Are there plans to support passing through the CancellationToken as indicated in the prior reply?

Thanks

@nils-a
Copy link
Contributor

nils-a commented Jan 31, 2022

@Simonl9l while there weren't any plans to implement CancellationTokens before this issue as far as I know, I personally see no reason not have them added in the future.

@Simonl9l
Copy link
Author

Simonl9l commented Jan 31, 2022

@nils-a are you able to set any expectation of plans on what "future" might mean?

The underlying value proposition is that Id want to hook the CLI into PosixSignalRegistration per here such that I can cancel a command execution based on a SIGTERM/INT etc..

TBH Im still getting the hang of the library and started using become for the Console Tables etc.

As an aside are the also any plans to support Tab/Command completion in the CLI?

@nils-a
Copy link
Contributor

nils-a commented Feb 1, 2022

are you able to set any expectation of plans on what "future" might mean?

Sadly, no. I find the idea intriguing, so I think I will look into it. Not sure when I have the cycles, though. And I'm even less sure as to when it might be "done".

As an aside are the also any plans to support Tab/Command completion in the CLI?

Have a look at #267

@Cryptoc1
Copy link

I recently encountered the need to support cancellation in my CLI, and wanted to share my solution/workaround.

In my case, SIGINT/SIGTERM would terminate the cli process, but some background work would continue running. The API managing this background work supported passing a CancellationToken, so my solution consist of a abstract implementation of AsyncCommand that uses a CancellationTokenSource to encapsulate the System.Console.CancelKeyPress and AppDomain.CurrentDomain.ProcessExit events:

using Spectre.Console.Cli;

namespace Cli;

public abstract class CancellableAsyncCommand : AsyncCommand
{
    public abstract Task<int> ExecuteAsync( CommandContext context, CancellationToken cancellation );

    public sealed override async Task<int> ExecuteAsync( CommandContext context )
    {
        using var cancellationSource = new CancellationTokenSource();

        System.Console.CancelKeyPress += onCancelKeyPress;
        AppDomain.CurrentDomain.ProcessExit += onProcessExit;

        using var _ = cancellationSource.Token.Register(
            ( ) =>
            {
                AppDomain.CurrentDomain.ProcessExit -= onProcessExit;
                System.Console.CancelKeyPress -= onCancelKeyPress;
            }
        );

        var cancellable = ExecuteAsync( context, cancellationSource.Token );
        return await cancellable;

        void onCancelKeyPress( object? sender, ConsoleCancelEventArgs e )
        {
            // NOTE: cancel event, don't terminate the process
            e.Cancel = true;

            cancellationSource.Cancel();
        }

        void onProcessExit( object? sender, EventArgs e )
        {
            if( cancellationSource.IsCancellationRequested )
            {
                // NOTE: SIGINT (cancel key was pressed, this shouldn't ever actually hit however, as we remove the event handler upon cancellation of the `cancellationSource`)
                return;
            }

            cancellationSource.Cancel();
        }
    }
}

When targeting NETCOREAPP directly a PosixSignalRegistration, as @Simonl9l suggests, makes for a simpler implementation:

using System.Runtime.InteropServices;
using Spectre.Console.Cli;

namespace Cli;

public abstract class CancellableAsyncCommand : AsyncCommand
{
    public abstract Task<int> ExecuteAsync( CommandContext context, CancellationToken cancellation );

    public sealed override async Task<int> ExecuteAsync( CommandContext context )
    {
        using var cancellationSource = new CancellationTokenSource();

        using var sigInt = PosixSignalRegistration.Create( PosixSignal.SIGINT, onSignal );
        using var sigQuit = PosixSignalRegistration.Create( PosixSignal.SIGQUIT, onSignal );
        using var sigTerm = PosixSignalRegistration.Create( PosixSignal.SIGTERM, onSignal );

        var cancellable = ExecuteAsync( context, cancellationSource.Token );
        return await cancellable;

        void onSignal( PosixSignalContext context )
        {
            context.Cancel = true;
            cancellationSource.Cancel();
        }
    }
}

With this approach, commands inherit from CancellableAsyncCommand, rather than AsyncCommand, and no changes should be needed to the Program.cs:

using Microsoft.Extensions.DependencyInjection;
using Spectre.Console.Cli;

var services = new ServiceCollection()
    .AddCliServices();

var registrar = new TypeRegistrar( services );
var app = new CommandApp<DefaultCommand>( registrar );

app.Configure(
    config =>
    {
#if DEBUG
        config.PropagateExceptions();
        config.ValidateExamples();
#endif
    }
);

return await app.RunAsync( args );

public class DefaultCommand : CancellableAsyncCommand
{
    public async Task<int> ExecuteAsync( CommandContext context, CancellationToken cancellation )
    {
        await DoWorkInThreads( cancellation );

        // ...
        return 0;
    } 
}

References:

P.S. I've just started using Spectre after using natemcmaster/CommandLineUtils for a while, and playing around with dotnet/command-line-api a bit; I think I'm in love.. ✌🏻❤️

@alvpickmans
Copy link

@Cryptoc1 nice workaround, it feels it would be a nice and simple addition to the library (yet I'm sure it has some implications)

I've decouple the cancellation token source from the abstract command implementation so it can be used on both AsyncCommand and AsyncCommand, but otherwise pretty nice solution for now!

internal sealed class ConsoleAppCancellationTokenSource 
{
    private readonly CancellationTokenSource _cts = new();

    public CancellationToken Token => _cts.Token;

    public ConsoleAppCancellationTokenSource()
    {
        System.Console.CancelKeyPress += OnCancelKeyPress;
        AppDomain.CurrentDomain.ProcessExit += OnProcessExit;

        using var _ = _cts.Token.Register(
            () =>
            {
                AppDomain.CurrentDomain.ProcessExit -= OnProcessExit;
                System.Console.CancelKeyPress -= OnCancelKeyPress;
            }
        );
    }

    private void OnCancelKeyPress(object? sender, ConsoleCancelEventArgs e)
    {
        // NOTE: cancel event, don't terminate the process
        e.Cancel = true;

        _cts.Cancel();
    }

    private void OnProcessExit(object? sender, EventArgs e)
    {
        if (_cts.IsCancellationRequested)
        {
            // NOTE: SIGINT (cancel key was pressed, this shouldn't ever actually hit however, as we remove the event handler upon cancellation of the `cancellationSource`)
            return;
        }

        _cts.Cancel();
    }
}
public abstract class CancellableAsyncCommand : AsyncCommand
{
    private readonly ConsoleAppCancellationTokenSource _cancellationTokenSource = new();

    public abstract Task<int> ExecuteAsync(CommandContext context, CancellationToken cancellation);

    public sealed override async Task<int> ExecuteAsync(CommandContext context)
        => await ExecuteAsync(context, _cancellationTokenSource.Token);

}

public abstract class CancellableAsyncCommand<TSettings> : AsyncCommand<TSettings>
    where TSettings : CommandSettings
{
    private readonly ConsoleAppCancellationTokenSource _cancellationTokenSource = new();

    public abstract Task<int> ExecuteAsync(CommandContext context, TSettings settings, CancellationToken cancellation);

    public sealed override async Task<int> ExecuteAsync(CommandContext context, TSettings settings)
        => await ExecuteAsync(context, settings, _cancellationTokenSource.Token);
}

@Simonl9l
Copy link
Author

Would be good to incorporate into core product, and also do simulate to create anValudateAsync path too.

@solonrice
Copy link

As another work-around is you could set up a ICommandInterceptor that adds a CancellationToken to the base settings if it is default. Then you have it in all your Settings parameters in every command.

@codymullins
Copy link

@patriksvensson do you need help with this issue? Think you'd like to move forward here?

@Cybrosys
Copy link

@Cryptoc1 Thank you for the CancellableAsyncCommand!

@github-actions github-actions bot added ⭐ top issue Top issue. ⭐ top feature Top feature request. labels Apr 15, 2024
0xced added a commit to 0xced/spectre.console that referenced this issue Sep 11, 2024
@0xced
Copy link
Contributor

0xced commented Sep 11, 2024

I have been injecting the cancellation token into the command but as @Simonl9l mentioned, it really feels very unconventional. Cancellation token really ought to be built-in.

So I have started working on my RunAsync-CancellationToken branch. It's not yet ready for a pull request because I have not yet updated the documentation and I still have many open questions that we can discuss here.

  • Do we really need/want a ValidateAsync method?
  • When the OperationCanceledException bubbles up to the CommandApp, how should it be handled? I have currently chosen to exit with a configurable CancellationExitCode which defaults to 130.
    • Is exiting with a specific code the right thing to do?
    • Is 130 a good default value and should it be different when running on Windows or Linux/macOS? (Here's some reading material)
  • Adding CancellationToken cancellationToken = default to the ICommand interface and thus the abstract Command, Command<TSettings>, AsyncCommand and AsyncCommand<TSettings> abstract classes is an annoying breaking change. Is there a way to mitigate this? In its current form, all command subclasses will fail to compile because of the added CancellationToken parameter.

Please have a look at my branch and let me know what you think.

0xced added a commit to 0xced/spectre.console that referenced this issue Sep 11, 2024
0xced added a commit to 0xced/spectre.console that referenced this issue Sep 11, 2024
0xced added a commit to 0xced/spectre.console that referenced this issue Sep 11, 2024
0xced added a commit to 0xced/spectre.console that referenced this issue Sep 12, 2024
Also raise CA2016 (forward the CancellationToken parameter to methods that take one) to warning

Fixes spectreconsole#701
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⭐ top feature Top feature request. ⭐ top issue Top issue.
Projects
Status: Todo 🕑
Development

No branches or pull requests

10 participants