From 789495f62ea8457b9685437385679b20a6bb2543 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 3 Jun 2025 15:24:36 +0200 Subject: [PATCH 1/3] add failing test --- .../Invocation/InvocationTests.cs | 37 ++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/src/System.CommandLine.Tests/Invocation/InvocationTests.cs b/src/System.CommandLine.Tests/Invocation/InvocationTests.cs index b855b4b905..1dc6c841e3 100644 --- a/src/System.CommandLine.Tests/Invocation/InvocationTests.cs +++ b/src/System.CommandLine.Tests/Invocation/InvocationTests.cs @@ -188,7 +188,42 @@ public async Task Anonymous_RootCommand_int_returning_Action_can_set_custom_resu (await rootCommand.Parse("").InvokeAsync()).Should().Be(123); } - + + [Fact] + public async Task Anonymous_async_action_is_not_mapped_into_sync_void_with_fire_and_forget() + { + RootCommand rootCommand = new(); + CancellationTokenSource cts = new(); + bool wasCancelled = false; + + Task cancellableDelay = Task.Delay(TimeSpan.FromHours(1), cts.Token); + + // We are not using the overload that takes a CancellationToken to ensure that + // we reproduce the behavior described in https://github.com/dotnet/command-line-api/issues/2562: + // having an "async void" action passed as Action to SetAction + // and running into "fire and forget". + rootCommand.SetAction(async parseResult => + { + try + { + await cancellableDelay; + } + catch (TaskCanceledException) + { + wasCancelled = true; + } + + }); + + Task started = rootCommand.Parse("").InvokeAsync(); + started.IsCompleted.Should().BeFalse("It should return a Task that has not finished yet."); + + cts.Cancel(); + await started; + wasCancelled.Should().BeTrue("It should be faulted because the delay was cancelled."); + started.IsCompleted.Should().BeTrue("It should complete after the cancellation."); + } + [Fact] public void Terminating_option_action_short_circuits_command_action() { From e11df2eeb3b2b03505faef6d0092b40436e3cd8e Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 3 Jun 2025 15:38:12 +0200 Subject: [PATCH 2/3] introduce a new overload that will be selected by the compiler hide it from intellisense, so the users are more likely to use the overload that takes CancellationToken --- ...ommandLine_api_is_not_changed.approved.txt | 1 + src/System.CommandLine/Command.cs | 24 +++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt index cf7c930638..d6319ed1be 100644 --- a/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt +++ b/src/System.CommandLine.ApiCompatibility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt @@ -54,6 +54,7 @@ public System.Void SetAction(System.Action action) public System.Void SetAction(System.Func action) public System.Void SetAction(System.Func action) + public System.Void SetAction(System.Func action) public System.Void SetAction(System.Func> action) public class CommandLineConfiguration .ctor(Command rootCommand) diff --git a/src/System.CommandLine/Command.cs b/src/System.CommandLine/Command.cs index e61b771b1a..7a1bc15941 100644 --- a/src/System.CommandLine/Command.cs +++ b/src/System.CommandLine/Command.cs @@ -149,6 +149,30 @@ public void SetAction(Func action) }); } + /// + /// Sets an asynchronous action to be run when the command is invoked. + /// + /// + /// When possible, prefer using the overload + /// and passing the parameter to the async method(s) called by the action. + /// + // Hide from intellisense, it's public to avoid the compiler choosing a sync overload + // for an async action (and fire and forget issue described in https://github.com/dotnet/command-line-api/issues/2562). + [EditorBrowsable(EditorBrowsableState.Never)] + public void SetAction(Func action) + { + if (action is null) + { + throw new ArgumentNullException(nameof(action)); + } + + Action = new AnonymousAsynchronousCommandLineAction(async (context, cancellationToken) => + { + await action(context); + return 0; + }); + } + /// /// Sets an asynchronous action when the command is invoked. /// From ac34c5a538843025ab3a616d6a97dbe10cfb66a4 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 4 Jun 2025 14:39:42 +0200 Subject: [PATCH 3/3] simplify the test --- .../Invocation/InvocationTests.cs | 33 +++++-------------- 1 file changed, 9 insertions(+), 24 deletions(-) diff --git a/src/System.CommandLine.Tests/Invocation/InvocationTests.cs b/src/System.CommandLine.Tests/Invocation/InvocationTests.cs index 1dc6c841e3..b9cc930bef 100644 --- a/src/System.CommandLine.Tests/Invocation/InvocationTests.cs +++ b/src/System.CommandLine.Tests/Invocation/InvocationTests.cs @@ -189,39 +189,24 @@ public async Task Anonymous_RootCommand_int_returning_Action_can_set_custom_resu (await rootCommand.Parse("").InvokeAsync()).Should().Be(123); } - [Fact] - public async Task Anonymous_async_action_is_not_mapped_into_sync_void_with_fire_and_forget() + [Fact] // https://github.com/dotnet/command-line-api/issues/2562 + public void Anonymous_async_action_is_not_mapped_into_sync_void_with_fire_and_forget() { RootCommand rootCommand = new(); - CancellationTokenSource cts = new(); - bool wasCancelled = false; - - Task cancellableDelay = Task.Delay(TimeSpan.FromHours(1), cts.Token); + using CancellationTokenSource cts = new(); + Task delay = Task.Delay(TimeSpan.FromHours(1), cts.Token); - // We are not using the overload that takes a CancellationToken to ensure that - // we reproduce the behavior described in https://github.com/dotnet/command-line-api/issues/2562: - // having an "async void" action passed as Action to SetAction - // and running into "fire and forget". - rootCommand.SetAction(async parseResult => + rootCommand.SetAction(async parseResult => { - try - { - await cancellableDelay; - } - catch (TaskCanceledException) - { - wasCancelled = true; - } - + await delay; }); Task started = rootCommand.Parse("").InvokeAsync(); - started.IsCompleted.Should().BeFalse("It should return a Task that has not finished yet."); + + // The action is supposed to wait for an hour, so it should not complete immediately. + started.IsCompleted.Should().BeFalse(); cts.Cancel(); - await started; - wasCancelled.Should().BeTrue("It should be faulted because the delay was cancelled."); - started.IsCompleted.Should().BeTrue("It should complete after the cancellation."); } [Fact]