Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
public System.Void SetAction(System.Action<ParseResult> action)
public System.Void SetAction(System.Func<ParseResult,System.Int32> action)
public System.Void SetAction(System.Func<ParseResult,System.Threading.CancellationToken,System.Threading.Tasks.Task> action)
public System.Void SetAction(System.Func<ParseResult,System.Threading.Tasks.Task> action)
public System.Void SetAction(System.Func<ParseResult,System.Threading.CancellationToken,System.Threading.Tasks.Task<System.Int32>> action)
public class CommandLineConfiguration
.ctor(Command rootCommand)
Expand Down
22 changes: 21 additions & 1 deletion src/System.CommandLine.Tests/Invocation/InvocationTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,27 @@ public async Task Anonymous_RootCommand_int_returning_Action_can_set_custom_resu

(await rootCommand.Parse("").InvokeAsync()).Should().Be(123);
}


[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();
using CancellationTokenSource cts = new();
Task delay = Task.Delay(TimeSpan.FromHours(1), cts.Token);

rootCommand.SetAction(async parseResult =>
{
await delay;
});

Task started = rootCommand.Parse("").InvokeAsync();

// The action is supposed to wait for an hour, so it should not complete immediately.
started.IsCompleted.Should().BeFalse();

cts.Cancel();
}

[Fact]
public void Terminating_option_action_short_circuits_command_action()
{
Expand Down
24 changes: 24 additions & 0 deletions src/System.CommandLine/Command.cs
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,30 @@ public void SetAction(Func<ParseResult, CancellationToken, Task> action)
});
}

/// <summary>
/// Sets an asynchronous action to be run when the command is invoked.
/// </summary>
/// <remarks>
/// When possible, prefer using the <see cref="SetAction(Func{ParseResult, CancellationToken, Task})"/> overload
/// and passing the <see cref="CancellationToken"/> parameter to the async method(s) called by the action.
/// </remarks>
// 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).
Comment thread
adamsitnik marked this conversation as resolved.
[EditorBrowsable(EditorBrowsableState.Never)]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you wanted to force folks to think about the CancellationToken you could also mark it obsolete -- not sure how hardcore you are about that.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It was my first idea, but then I realized that we should most likely not release new public [Obsolete] methods. But I agree, it would be even better as it would allow us emit a compiler warning/error with a clear message.

@jonsequitur thoughts?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it be simpler to differentiate the methods by name rather than using overloads, e.g. SetAction and SetAsyncAction?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Would it be simpler to differentiate the methods by name rather than using overloads, e.g. SetAction and SetAsyncAction?

I like the SetAsyncAction name idea (we never discussed actions at the API review), but users could still run into this particular bug and it would cause another wave of breaking changes in SDK/VMR etc. Let's discuss it offline, I think it's now or never.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

we should most likely not release new public [Obsolete] methods

dotnet/Open-XML-SDK#1600 added ObsoleteAttribute to several new types and methods. There though, they intend to remove the attribute eventually, unlike here.

public void SetAction(Func<ParseResult, Task> action)
{
if (action is null)
{
throw new ArgumentNullException(nameof(action));
}

Action = new AnonymousAsynchronousCommandLineAction(async (context, cancellationToken) =>
{
await action(context);
return 0;
});
}

/// <summary>
/// Sets an asynchronous action when the command is invoked.
/// </summary>
Expand Down