Separate CliAction into synchronous and asynchronous types - #2205
Conversation
| where THandler : CliAction | ||
| { | ||
| command.Action = CommandHandler.Create(typeof(THandler).GetMethod(nameof(CliAction.InvokeAsync))); | ||
| command.Action = CommandHandler.Create(typeof(THandler).GetMethod(nameof(AsynchronousCliCommandAction.InvokeAsync))); |
There was a problem hiding this comment.
This seems incompatible with explicit override supported by Visual Basic, i.e. where THandler has a method that overrides AsynchronousCliCommandAction.InvokeAsync but is not itself named InvokeAsync.
Besides, GetMethod will fail due to ambiguity if THandler has multiple public methods named InvokeAsync, which is possible in C# too.
There was a problem hiding this comment.
This code is related to the Hosting package's dependency on NamingConventionBinder. Since neither of these packages is planned for GA at this time and the use of reflection will be completely replaced, it's unlikely we'll fix these.
There was a problem hiding this comment.
Do you intend to keep publishing those packages to NuGet as preview versions? And DragonFruit, I guess.
There was a problem hiding this comment.
There's been no decision but it's unlikely.
DragonFruit will be superseded the source generator work that's planned as a followup to the System.CommandLine GA.
| switch (action) | ||
| { | ||
| case SynchronousCliAction syncAction: | ||
| syncAction.Invoke(parseResult); |
There was a problem hiding this comment.
If cancellation is triggered by console ^C, then it would be useful to inform sync actions about it, too.
There was a problem hiding this comment.
If cancellation is triggered by console ^C, then it would be useful to inform sync actions about it, too.
How? Any why anyone should use sync overload to get cancellation support which is one of the main reasons of using async?
There was a problem hiding this comment.
CPU-bound work but with temporary files and directories that should be deleted when canceled.
There was a problem hiding this comment.
I was looking for a place to put the same comment @KalleOlaviNiemitalo 's puts here. It is reasonable to have sync that supports cancellation via events. If we do not support that, we should make a clear decision on preferring the benefit of a single cancellation strategy to the benefit of not having to mess with tasks when they are otherwise unnecessary.
There was a problem hiding this comment.
Also, synchronous code tends to result in more understandable stack traces.
There was a problem hiding this comment.
It's also reasonable to use CancellationToken in synchronous APIs. The idea was suggested by API review.
There was a problem hiding this comment.
CPU-bound work but with temporary files and directories that should be deleted when canceled.
This can be achieved with FileOptions.DeleteOnClose which deletes the file when the handle is released.
It's also reasonable to use
CancellationTokenin synchronous APIs.
It's definitely possible, but I am not sure whether the users would be able to take advantage of that.
With async code, everyone just needs to propagate the token to the async methods they are using and the compiler emits a warning when they don't.
At some point of time, the token is passed to one of the System* methods that implement cancellation: they monitor the change of the token state and when it's requested, they perform actual cancellation by for example asking the OS to cancell given IO operaiton.
With sync code, the users would be left on their own. They would need to constantly monitor the status of the token, and when cancellation is requested, handle it on their own and throw an exception.
But they could interrupt only some of the operations, namely only their own CPU-bound computations. They could not cancel IO operations, as they would be using the sync ones. Or at least they should, as sync should not be mixed with async. If they end up calling async methods from sync methods just to pass the cancellation token and perform blocking wait on the task, the design leads them into failure.
So it's possible, but I don't believe that it's the right direction as it would increase complexity and almost never provide the required value.
There was a problem hiding this comment.
CPU-bound work but with temporary files and directories that should be deleted when canceled.
This can be achieved with
FileOptions.DeleteOnClosewhich deletes the file when the handle is released.
This is just one example. Not all resources (e.g. processes, cloud resources) will have APIs for automatic cleanup.
There was a problem hiding this comment.
POSIX does not support a feature like FileOptions.DeleteOnClose. The SafeFileHandle.ReleaseHandle implementation apparently works around that by checking the flag and deleting the file; but how can the application ensure that ReleaseHandle is called in response to SIGINT? That would require either calling Close/Dispose on the FileStream, or clearing references and running GC; either way, this needs to be supported by the code that owns the streams. It seems passing in a CancellationToken is the best way, even in synchronous code.
There is no DirectoryOptions.DeleteOnClose for temporary directories, either.
| startedInvocation = asyncAction.InvokeAsync(parseResult, cts.Token); | ||
| break; | ||
| default: | ||
| throw new ArgumentOutOfRangeException(nameof(parseResult.Action)); |
There was a problem hiding this comment.
The exception seems bogus as there is no Action parameter. There was a private protected constructor; is the default branch reachable only if parseResult.Action is null? If so, I suggest just skipping the call, like in the previous switch on nonexclusive actions.
There was a problem hiding this comment.
It was here more to satisfy the compiler about the lack of a default return value. I'll look for ways to refactor away from what should be an unreachable code path.
| parent.Children.Where(r => !(r is OptionResult optionResult && optionResult.Option is VersionOption)) | ||
| .Any(NotImplicit)) | ||
| parent.Children.Any(r => r is not OptionResult { Option: VersionOption })) |
There was a problem hiding this comment.
Why isn't NotImplicit needed any more?
There was a problem hiding this comment.
It was only used here, inlined, and simplified.
There was a problem hiding this comment.
I don't see how it was inlined. Should the call be parent.Children.Any(r => r is not OptionResult { Option: VersionOption, Implicit: false })?
There was a problem hiding this comment.
The VersionOption is never implicit.
Code coverage indicated that this code could be inlined so I inlined it. I spent a little time trying to write a test that proves this change introduces a bug, but I haven't found anything so far. Any ideas?
adamsitnik
left a comment
There was a problem hiding this comment.
I added some comments, overall it looks much better than I imagined! 👍
|
|
||
| public abstract class CliAction | ||
| { | ||
| private protected CliAction() |
There was a problem hiding this comment.
is it enough to allow for inheriting from CliAction to only System.CommandLine? Nice!
There was a problem hiding this comment.
Not having gotten to those classes in the review, but I assume folks can create their own Synchronous and Asynchronous actions.
There was a problem hiding this comment.
Yes. What I'd like to enforce is that no one can create a CliAction that isn't either a SynchronousCliAction or AsynchronousCliAction.
| public abstract Task<int> InvokeAsync(ParseResult parseResult, CancellationToken cancellationToken = default); | ||
| } | ||
|
|
||
| public class SynchronousCliCommandAction : SynchronousCliAction |
There was a problem hiding this comment.
what do we gain by exposing this type?
also: by naming it Command we limit the usage to commands only, while we may want to use it for directive or active options one day too
| public class SynchronousCliCommandAction : SynchronousCliAction | |
| internal class SynchronousCliCommandAction : SynchronousCliAction |
There was a problem hiding this comment.
Maybe I do not understand the usage, but it does not look like folks can create their own actions, and I recalled that as a key use case.
There was a problem hiding this comment.
what do we gain by exposing this type?
also: by naming it Command we limit the usage to commands only, while we may want to use it for directive or active options one day too
This is the goal -- to enable people to distinguish between command actions and other kinds of actions, which tend to represent interception of some kind, whether by a directive, help, or a parse error.
There was a problem hiding this comment.
Maybe I do not understand the usage, but it does not look like folks can create their own actions, and I recalled that as a key use case.
They can. There are a number of examples in the tests.
There was a problem hiding this comment.
This is the goal -- to enable people to distinguish between command actions and other kinds of actions, which tend to represent interception of some kind, whether by a directive, help, or a parse error.
Just thinking out loud: maybe we should consider exposing Symbol Origin in the CliAction to allow for such checks?
Then we would need to expose fewer public types and the checks in theory would be simpler (knowing about the existance of both CliCommandAction types)?
- bool isCommandAction = parseResult.Action is SynchronousCliCommandAction or AsynchronousCliAction;
+ bool isCommandAction = parseResult.Action.Origin is Command;The tricky thing would be choosing the right symbol for ParseErrorAction (it could be always root command, just null or maybe the symbol that produced errors if it was the only one with parse issues)
PS. Origin may be a bad name, I hope you get my intentions.
There was a problem hiding this comment.
The tricky thing would be choosing the right symbol for ParseErrorAction
A ParseErrorAction can't reliably be associated with a specific symbol.
There was a problem hiding this comment.
Part of why we have the extra types here is that the previous concept of an "anonymous" CliAction has been merged with the CliCommandAction, which is probably the wrong thing to do. The anonymous pattern (used by the SetHandler methods to create a CliAction instance whose implementation is injected via a delegate) shouldn't be coupled to whether the action is related to a command.
I can remove the concept of a command action. The check for whether it's the intended action can be satisfied like this:
if (parseResult.Action != expectedCommand.Action) { }Pattern matches will have to based on very specific types rather than base types.
Anonymous action types (as internal types) will probably need to be put back in place.
I'll make these changes and we can see how it looks.
| switch (action) | ||
| { | ||
| case SynchronousCliAction syncAction: | ||
| syncAction.Invoke(parseResult); |
There was a problem hiding this comment.
If cancellation is triggered by console ^C, then it would be useful to inform sync actions about it, too.
How? Any why anyone should use sync overload to get cancellation support which is one of the main reasons of using async?
| switch (parseResult.Action) | ||
| { | ||
| case SynchronousCliAction syncAction: | ||
| startedInvocation = Task.FromResult(syncAction.Invoke(parseResult)); |
There was a problem hiding this comment.
We should most probably (I am not 100% sure) call Task.Run here and provide it with the cancellation token if we want to get cancellation to work, otherwise we are going to:
- perform a blocking call and wait until it's finished
- register for cancellation and do all other things that won't matter as the job will be already finished
| startedInvocation = Task.FromResult(syncAction.Invoke(parseResult)); | |
| startedInvocation = Task.Run(() => syncAction.Invoke(parseResult), cts.Token); |
There was a problem hiding this comment.
From Task.Run(Action, CancellationToken) docs:
A cancellation token allows the work to be cancelled if it has not yet started.
but here it's likely to start immediately (thread pool is not so busy that it starts throttling creation of new threads) so the CancellationToken would have little effect.
There was a problem hiding this comment.
Also -- if the Main method has STAThreadAttribute but you let Task.Run post the action to the thread pool, then it won't be in the STA, and this could break Clipboard operations.
There was a problem hiding this comment.
The goal here is to avoid the extra task overhead for work that's not async.
There was a problem hiding this comment.
If we don't start task of some kind for performing the synchronous action, the code below will be pointless, as it's going to register for cancellation after work has been finished:
- Perform blocking action call and wait until it's finished
- Register for cancellation
- Return immediately from
Task.WhenAnyas the work has been already done
If the sync call never finishes, we won't be able to cancel it gracefully.
If for some reason we don't want to support cancellation in this scenario (I doubt), we should simple return here
- startedInvocation = Task.FromResult(syncAction.Invoke(parseResult));
+ return syncAction.Invoke(parseResult);There was a problem hiding this comment.
It's up to the handler (the developer's code) to implement cancellation, not the caller (our code here). For example, someone could do this inside a synchronous handler (assuming we figure out how to provide the cancellation token):
while (!cancellationToken.IsCancellationRequested && !DoneDoingWork())
{
// do stuff
}
CleanUp();There was a problem hiding this comment.
Create the ProcessTerminationHandler before the parseResult.NonexclusiveActions loop, so that it can translate Console.CancelKeyPress to CancellationTokenSource.Cancel() even during the synchronous part of AsynchronousCliAction.InvokeAsync.
The CancellationToken can then be passed to SynchronousCliAction.Invoke as well. If SynchronousCliAction.Invoke throws OperationCanceledException and cancellation was requested via ProcessTerminationHandler, InvocationPipeline should return the exit code chosen by ProcessTerminationHandler, rather than the hardcoded 1 in DefaultExceptionHandler; this would require some extra code.
There was a problem hiding this comment.
This is a good suggestion. For completeness we would also need to implement this in the InvocationPipeline.Invoke path.
| syncAction.Invoke(parseResult); | ||
| break; | ||
| case AsynchronousCliAction _: | ||
| throw new InvalidOperationException($"{nameof(AsynchronousCliAction)} called within non-async invocation."); |
There was a problem hiding this comment.
I am big supporter of throwing in such case. 🥇
My only suggestion would be to throw when the synchronous invocation starts and we detect that a command with async action is in the symbol tree (so when user has n sync commands and only one async, they are always going to get the exception and immediately switch to InvokeAsync)
There was a problem hiding this comment.
Good point. It might be though that this code is unreachable and better suited as a Debug.Assert. This method is only called currently from ParseResult.Invoke which does sync-over-async if it detects that there are any synchronous actions.
There was a problem hiding this comment.
I enclosed this in a #if DEBUG block, since this state is currently unreachable via the public API.
KathleenDollard
left a comment
There was a problem hiding this comment.
Some small comments. Overall, many smiles here.
|
|
||
| public abstract class CliAction | ||
| { | ||
| private protected CliAction() |
There was a problem hiding this comment.
Not having gotten to those classes in the review, but I assume folks can create their own Synchronous and Asynchronous actions.
| public abstract Task<int> InvokeAsync(ParseResult parseResult, CancellationToken cancellationToken = default); | ||
| } | ||
|
|
||
| public class SynchronousCliCommandAction : SynchronousCliAction |
There was a problem hiding this comment.
Maybe I do not understand the usage, but it does not look like folks can create their own actions, and I recalled that as a key use case.
| switch (action) | ||
| { | ||
| case SynchronousCliAction syncAction: | ||
| syncAction.Invoke(parseResult); |
There was a problem hiding this comment.
I was looking for a place to put the same comment @KalleOlaviNiemitalo 's puts here. It is reasonable to have sync that supports cancellation via events. If we do not support that, we should make a clear decision on preferring the benefit of a single cancellation strategy to the benefit of not having to mess with tasks when they are otherwise unnecessary.
| /// </summary> | ||
| /// <returns>A value that can be used as a process exit code.</returns> | ||
| public int Invoke() => InvocationPipeline.Invoke(this); | ||
| public int Invoke() |
There was a problem hiding this comment.
No description provided.
5f33967 to
6bfa687
Compare
80f68d6 to
b2caf06
Compare
|
|
||
| namespace System.CommandLine.Invocation; | ||
|
|
||
| internal class AnonymousSynchronousCliAction : SynchronousCliAction |
There was a problem hiding this comment.
nit: it should be sealed as we don't expect any types to derive from it
| internal class AnonymousSynchronousCliAction : SynchronousCliAction | |
| internal sealed class AnonymousSynchronousCliAction : SynchronousCliAction |
| { | ||
| private CliAction? _action; | ||
|
|
||
| /// <inheritdoc /> |
There was a problem hiding this comment.
I am not sure whether inheritdoc is the best choice here, as it's going to provide generic information about the base type
| /// <summary> | ||
| /// Configures the application to provide alternative suggestions when a parse error is detected. Disabled by default. | ||
| /// </summary> | ||
| public bool EnableTypoCorrections { get; set; } = false; |
There was a problem hiding this comment.
The disadvantage of moving different configuration possibilities from CliConfiguration to dedicated action types is that it makes them harder to discover.
With one config type I have everything in one place and it's quite easy to discover about the existence of the type (the Command.Parse takes an optional argument of this type).
When we move the config knobs to dedicated action types it becomes harder, as the end users need to find out about the existence of these dedicated action types. In this regard we can only rely on IDE suggestions?
The advantage is that we can be more selective. In case of the typo correction, we can make it effective for specific command rather that to all of them. (BTW SDK implemented the typo corrections for one specific command on their own..)
There was a problem hiding this comment.
SDK implemented typo corrections ourselves mostly because we couldn't enable it across all-commands. The SDK is setup kind of oddly
- we parse input args
- if the result is invokable (meaning it found an actual command to run), invoke it and return
- otherwise, try to see if the command is actually a .NET SDK tool (local or global), and if so run that
- finally, bail out if no command was found
Since we do this odd two-phase execution, middleware like typo correction never actually got a chance to be executed. So instead we implemented it for the template engine because the team wanted to provide a better user experience for typo'd template and option names. It is something I'd like to have on globally, but we have some architectural blockers there.
There was a problem hiding this comment.
With one config type I have everything in one place and it's quite easy to discover about the existence of the type (the Command.Parse takes an optional argument of this type).
The problem is that behavioral configurations like this are going to be added by other packages over time, and these won't be found on the configuration. The actual implementation is on the CliSymbol.Action. This is why we've discussed focusing CliConfiguration on parser behaviors, not on invocation behaviors.
We can still potentially make them discoverable from the CliConfiguration using extension methods that dig up the appropriate option and configure its actions from there, e.g. config.EnableTypeCorrections() (as an extension method) is trivial to implement on top of this current API.
| /// <summary> | ||
| /// Indicates that the action terminates a command line invocation, and later actions are skipped. | ||
| /// </summary> | ||
| public bool Terminating { get; protected set; } = true; |
There was a problem hiding this comment.
The current design makes it possible to change the value during actual invocation.
Do we want to allow for that? Or should we make it readonly?
There was a problem hiding this comment.
Middleware allowed for conditionally short-circuiting which is why I made this mutable, though I didn't test that it will actually work and suspect it won't, so making it readonly for now, until or unless we clarify the scenario, is probably a good idea.
There was a problem hiding this comment.
Changing this to protected init.
There was a problem hiding this comment.
Will it be possible to initialise that in application code that is built in C# 7? As C# 8 is not supported on .NET Framework.
There was a problem hiding this comment.
Tested; I cannot directly initialize a get; init; property using C# 7 on Visual Studio 2017.
In a class library built for netstandard2.0 using C# 9 on .NET SDK 7.0.105:
public class Class1
{
public bool Prop { get; protected init; }
}In an application built for net48 using C# 7 on Visual Studio 2017, referencing the DLL built from the class library project:
class Act : Init.Class1
{
public Act() : base()
{
this.Prop = true;
}
}error CS1545: Property, indexer, or event 'Class1.Prop' is not supported by the language; try directly calling accessor methods 'Class1.get_Prop()' or 'Class1.set_Prop(?)'
and if I try this.set_Prop(true) instead, then:
error CS0570: 'Class1.set_Prop(?)' is not supported by the language
If I avoid Visual Studio 2017 (which I would not be able to do in production) and instead use .NET SDK 7.0.105 to build the application too:
error CS8107: Feature 'init-only setters' is not available in C# 7.0. Please use language version 9.0 or greater.
and C# 9.0 is not supported on .NET Framework. So it is not possible to directly initialize this property in a project targeting .NET Framework, while retaining Microsoft support.
I suppose it would be possible to work around this limitation by initializing by reflection instead, but can you just change it back to get; protected set;? And preferably also delete src/System.CommandLine/System.Runtime.CompilerServices/IsExternalInit.cs to flag any other uses of init.
There was a problem hiding this comment.
No. We can revisit this implementation but I'd prefer not to block this PR on it as there are larger moving parts.
| public abstract class CliAction | ||
| public System.Boolean Terminating { get; } | ||
| protected System.Void set_Terminating(System.Boolean value) | ||
| public class ParseErrorAction : SynchronousCliAction |
There was a problem hiding this comment.
Just thinking out loud here: should we keep all actions in the System.CommandLine.Invocation namespace?
For example to me ParseErrorAction belongs in System.CommandLine.Parsing as it's related to parsing.
There was a problem hiding this comment.
My thought here was that since the entire invocation space is a effectively a different layer and could be replaced with a completely different invocation infrastructure on top of the same parser, none of these belong under parsing.
| /// <summary> | ||
| /// Provides command line output with error details in the case of a parsing error. | ||
| /// </summary> | ||
| public sealed class ParseErrorAction : SynchronousCliAction |
There was a problem hiding this comment.
I love the idea of merging TypoCorrectionAction into this type. Not because the number of types has lowered, but because it feels more natural (corrections can be only printed when there was a parse error). 🥇
There was a problem hiding this comment.
Thanks! I thought this worked nicely as well, and is a good example of how moving behavioral functionality out of the CliConfiguration can make things more cohesive.
| { | ||
| var useAsync = false; | ||
|
|
||
| if (Action is AsynchronousCliAction) |
There was a problem hiding this comment.
I strongly believe that we should be throwing in such scenarios. If any of the symbols defined in the symbol tree have an async action, ParseResult.Invoke should throw and point people to InvokeAsync. This make it very easy to fix on the user side and helps us avoid plenty of confusions and bugs in the future.
| switch (action) | ||
| { | ||
| case SynchronousCliAction syncAction: | ||
| syncAction.Invoke(parseResult); |
There was a problem hiding this comment.
CPU-bound work but with temporary files and directories that should be deleted when canceled.
This can be achieved with FileOptions.DeleteOnClose which deletes the file when the handle is released.
It's also reasonable to use
CancellationTokenin synchronous APIs.
It's definitely possible, but I am not sure whether the users would be able to take advantage of that.
With async code, everyone just needs to propagate the token to the async methods they are using and the compiler emits a warning when they don't.
At some point of time, the token is passed to one of the System* methods that implement cancellation: they monitor the change of the token state and when it's requested, they perform actual cancellation by for example asking the OS to cancell given IO operaiton.
With sync code, the users would be left on their own. They would need to constantly monitor the status of the token, and when cancellation is requested, handle it on their own and throw an exception.
But they could interrupt only some of the operations, namely only their own CPU-bound computations. They could not cancel IO operations, as they would be using the sync ones. Or at least they should, as sync should not be mixed with async. If they end up calling async methods from sync methods just to pass the cancellation token and perform blocking wait on the task, the design leads them into failure.
So it's possible, but I don't believe that it's the right direction as it would increase complexity and almost never provide the required value.
|
|
||
| ParseResult parseResult = command.Parse("cmd --1 true --3 false --2 true"); | ||
|
|
||
| using var _ = new AssertionScope(); |
There was a problem hiding this comment.
Using _ as the name of a variable rather than as a discard (C# would not allow another using var _ in the same scope) feels suspect to me.
There was a problem hiding this comment.
This is a common coding convention that predates discard support in the language.
Other suggestions?
This also adds properties to the ParseErrorAction to allow toggling help and typo suggestions when there's a parse error.
cf40b75 to
008524c
Compare
| internal static void SetHandlers(CliCommand command, Func<string[], IHostBuilder> hostBuilderFactory, Action<IHostBuilder> configureHost) | ||
| { | ||
| command.Action = new HostingAction(hostBuilderFactory, configureHost, command.Action); | ||
| command.Action = new HostingAction(hostBuilderFactory, configureHost, (AsynchronousCliAction)command.Action); |
There was a problem hiding this comment.
This breaks things for me, as the command.Action is either a HelpAction or AnonymousSynchronousCliOption giving me InvalidCastExceptions. Is UseHost() going to be limited to just async actions now?
This PR separates
CliActioninto two types:SynchronousCliActionandAsynchronousCliAction. This enables implementers to implement only one handler.Additional changes are in place to make certain
CliActiontypes public in order to start moving configuration properties that relate to invocation from theCliConfigurationclass to the specificCliAction-derived classes, with the goal of making this code more cohesive.