How do we implement exception handling around Command.Invoke?

[SuperJMN]

I have created a simple .NET Core console application with this line to execute the Command

try
{   
    rootCommand.Invoke(args)
}
catch 
{
    Console.Error.Write("Something bad happened");
}

But when the command handler associated to the command throws an exception, the catch block isn't executed. Instead, the Invoke method returns a non-zero integer.

So, what's the correct method to handle exceptions around Invoke/InvokeAsync?

[jonsequitur]

When you invoke a command directly like this, it gets wrapped with the default middlewares including UseExceptionHandler. This is intended to handle exceptions very generally but if you want to handle more specific exceptions, you should do so inside your command handler, which gets called after the middleware.

[AndrewSav]

Yes, this is something that I tripped on too. In my opinion printing the stack trace to the end user by default is not reasonable. Asking to create a separate exception handler for each command does not seem to conform to DRY either. I do not expect any library to catch and swallow my exception, I prefer to process them myself. A helper option like UseExceptionHandler is nice, but that should not be the default.

The workaround seems to be:

var _ = new CommandLineBuilder(rootCommand)
       .UseVersionOption()
       .UseHelp()
       .UseEnvironmentVariableDirective()
       .UseParseDirective()
       .UseDebugDirective()
       .UseSuggestDirective()
       .RegisterWithDotnetSuggest()
       .UseTypoCorrections()
       .UseParseErrorReporting()
       .CancelOnProcessTermination()
       .Build();

       rootCommand.Invoke(args);

Note, that the result of the CommandLineBuilder is discarded, it is stored in internal ImplicitParser property on the command object itself. Not very intuitive, but I guess could be documented?

[ohadschn]

Agree with @AndrewSav - IMHO UseExceptionHandler should not be on by default (or at the very least have a simple flag to turn it off), I prefer to process my exceptions too. As one example, I like letting fatal exceptions (like out of memory) pass through and crash the process.

[jonsequitur]

@AndrewSav Typically if you're wiring up the CommandLineBuilder our assumption is you'll call Invoke on the parser returned by its Build method rather than using the Command.Invoke extension method.

[jonsequitur]

As a middleware, UseExceptionHandler applies to all commands, so it satisfies DRY. You can customize it by passing your own delegate if you want to change the default behaviors, which are:

1. Sets the result code to 1.
2. Prints a stack trace to Console.Error.

We're open to suggestions that change these defaults. So far, printing the stack trace is the most useful generalizable approach we've seen suggested. Here's the reasoning. Your program crashed in some way your code didn't anticipate. An uninformative general message means that in order for you to investigate, either 1) your user would have to debug the program or 2) you would need to implement telemetry. The stack trace is trivial for your user to copy and paste into an issue, or search for online in order to find solutions. If the exception handler isn't in place, the just-in-time debugger will kick in for users who have Visual Studio installed. This is also more likely to happen to your users than to you, and they probably won't debug your code.

Do you have a preferred behavior in mind?

[AndrewSav]

As a middleware, UseExceptionHandler applies to all commands, so it satisfies DRY.

Sorry, my comment regarding DRY was specifically in response to suggestion of catching exceptions in command handlers. This was not with regards to the middleware.

We're open to suggestions that change these defaults. So far, printing the stack trace is the most useful generalizable approach we've seen suggested.

The suggestion was to not enable this middleware by default. Once one has decided that they want to use the middleware, the in-middleware defaults are indeed reasonable - I did not object to these.

[jonsequitur]

Not enabling it by default means the default behavior on an unhandled exception is a process crash, a stack trace, and possible triggering of the just-in-time debugger. It's also not a great experience, and perhaps slightly less useful.

[AndrewSav]

I disagree. CommandLine is a library, in my opinion a library should not catch exceptions it did not produce. If you change the handler to only catch exception that may have originate from CommandLine library itself, that would make certain sense. Swallowing unrelated exceptions should not happen.

There are also always "generic" catastrophic failure exceptions such as out of memory condition, those in my opinion should not be swallowed either even if they were caused by an allocation within the library.

I'm not alone. Phil Haack wrote in a comment to this Hanselman's page:

I'd probably mention that there is a difference between how you handle exceptions when writing a library vs writing an application. For example, a library should generally just propagate exceptions unless it's absolutely sure it can handle it in the best way possible.

I think @ohadschn and @SuperJMN above would also agree with this sentiment.

[jonsequitur]

It's often said that the difference between a library and a framework is that you call a library whereas a framework calls you. By that definition anyway, some parts of System.CommandLine are library (e.g. the parser) and some are framework (e.g. handler invocation). It's influenced by ASP.NET which, like many frameworks, has a top-level catch-all exception handler. The purpose of this is to handle exceptions that would affect the user experience in the cases when they were not handled by the developer.

It's also the case that the default exception handler behavior was based on user feedback that a crash is not a good default user experience. Knowing we can't design something that's perfect for every use case, we made the API composable. You can opt out of the default exception handling behavior by not calling UseDefaults or UseExceptionHandler.

If you have suggestions to improve the default experience, we want to hear them along with your opinion on why it's an improvement. We do have a bias toward user experience, and I think that's the piece I'm missing. How does removing the default exception handler improve user experience?

It also might be worth opening a separate issue, as @SuperJMN was asking a question and not, as far as I can tell, suggesting a design change.

[AndrewSav]

@jonsequitur, thank you for engaging in this discussion, I really appreciate your time. I feel that we have different ideological priorities, which will not be aligned in this thread. It appears that you have given a careful consideration to the issues that has not changed the outcome.

Command Line parsing has always been a bit of a problem in the .Net world. There has been many attempts to make it easier by different library, and none took on enough to become the mainstream. I was hopeful when CommandLine first emerged as I saw this as an opportunity to finally get it right. I wish it to succeed and make life of countless developers easier.

If you have suggestions to improve the default experience, we want to hear them along with your opinion on why it's an improvement. We do have a bias toward user experience, and I think that's the piece I'm missing. How does removing the default exception handler improve user experience?

When we talk about CommandLine parsing library, what it does is helping a developer to spend less time on a boilerplate code. Writing a good command line parsing library is surprisingly difficult, I would identify the main challenge that it's a futile attempt to make it cater for all possible command line syntax's, yet it should not make a developer feel shoehorned into a particular one that does not match his need. I think CommandLine strikes a good balance here.

In my mind the focus is on the developer experience not on the end-user experience. We have developers to care for end-user experience, so let's leave it with them. Let's empower them to provide that experience quickly and easily. We, as the library designers will focus on developers experience, so that developers could easily deliver top notch user experience.

Let's say I as a developer have a code that throws and unhanded exception under some circumstances. Is this a good user experience? Perhaps not. So to help that I catch this exception and handle it somewhere. Now I realised that my quick and dirty home-grown command line parser is inadequate, so I quickly plug-in CommandLine. This is just a library, so I do not expect it to interfere with exception handling. And yet it does!

My developer experience is not very good, It is not obvious at all what's happening or why, and it's not obvious how to work around that.

User experience has changed to worse an exception used to be nicely handled, and now it is dumped on screen and scary the user with technical stuff they do not understand.

In my mind it's pretty clear that this should not be the default. Even in asp.net framework I was not happy with that default, but for CommandLine, in my opinion it's far out.

But I do understand that you don't see it this way, and it does not appear that I can persuade you. So I'll leave it at this. Once again, thank you for contributing to this discussion, it's much appreciated.

[ohadschn]

Agree with @AndrewSav and IMHO System.CommandLine is not a framework (I don't think having a handler makes it one).

At any rate, I think we can all live with the current default, as long as it's easy to opt out of it (and keep all the other good default stuff). Specifically, it has to be easier than:

var _ = new CommandLineBuilder(rootCommand)
       .UseVersionOption()
       .UseHelp()
       .UseEnvironmentVariableDirective()
       .UseParseDirective()
       .UseDebugDirective()
       .UseSuggestDirective()
       .RegisterWithDotnetSuggest()
       .UseTypoCorrections()
       .UseParseErrorReporting()
       .CancelOnProcessTermination()
       .Build();

Not just easier, but immune to future default changes (e.g. added directives and whatnot). Something that works with the most basic sample program like:

rootCommand.Handler = CommandHandler.Create<int>(handleExceptions: false, (intOption) =>
    {
        Console.WriteLine($"The value for --int-option is: {intOption}");
    });

Or have it at the Command level, or have some static option to disable it, or something...

[jnm2]

I would really like to be able to handle exceptions myself without 1) repeating the handling in the root of each invoked command or 2) writing more than one line of code to opt out of the default behavior.

The default printing of several pages of red text full of ExceptionDispatchInfo boilerplate is not what I want. Especially (but not only) given that "graceful" Ctrl+C cancellation causes these pages of text.

[jnm2]

This is what I'm doing to minimize repetition. I still have to repeat the .WithExceptionHandler(ExceptionHandler) call once per subcommand, but it's better than hardcoding what the CommandLineBuilder defaults were at this point in time:

Click to expand

/// <summary>
/// Workaround for <see href="https://github.com/dotnet/command-line-api/issues/796"/>.
/// </summary>
public static ICommandHandler WithExceptionHandler(this ICommandHandler commandHandler, Func<Exception, int> exceptionHandler)
{
    return new ExceptionHandlingCommandHandler(commandHandler, exceptionHandler);
}

private sealed class ExceptionHandlingCommandHandler : ICommandHandler
{
    private readonly ICommandHandler commandHandler;
    private readonly Func<Exception, int> exceptionHandler;

    public ExceptionHandlingCommandHandler(ICommandHandler commandHandler, Func<Exception, int> exceptionHandler)
    {
        this.commandHandler = commandHandler;
        this.exceptionHandler = exceptionHandler;
    }

    public async Task<int> InvokeAsync(InvocationContext context)
    {
        try
        {
            return await commandHandler.InvokeAsync(context);
        }
        catch (Exception ex)
        {
            return exceptionHandler.Invoke(ex);
        }
    }
}

[jonsequitur]

Any suggestions on an API for opting out of this?

A related problem that needs to be solved is how extension methods like Command.Invoke should behave, given that under the hood they use UseDefaults so that people don't need to use the CommandLineBuilder.

@jnm2, I'm curious about why you implemented a custom ICommandHandler instead of a middleware.

[jnm2]

@jonsequitur I haven't figured out middleware yet. It's odd that you wouldn't be able to add middleware to a root command using an API exposed on the root command instance.

[jnm2]

Can we have a Configuration property on the root command and set UseDefaultExceptionHandler = false?

[jonsequitur]

The Symbol types don't know anything about the details of invocation (other than having a pointer to a handler). We wouldn't want to duplicate invocation functionality into Command.

The invocation, including the current exception handling, is all implemented in middleware, which gets called before and after the command handler is invoked.

The current default exception handling is implemented like this:

        public static CommandLineBuilder UseExceptionHandler(
            this CommandLineBuilder builder,
            Action<Exception, InvocationContext>? onException = null)
        {
            builder.AddMiddleware(async (context, next) =>
            {
                try
                {
                    await next(context);
                }
                catch (Exception exception)
                {
                    (onException ?? Default)(exception, context);
                }
            }, MiddlewareOrderInternal.ExceptionHandler);

            return builder;

            void Default(Exception exception, InvocationContext context)
            {
                context.Console.ResetTerminalForegroundColor();
                context.Console.SetTerminalForegroundRed();

                context.Console.Error.Write("Unhandled exception: ");
                context.Console.Error.WriteLine(exception.ToString());

                context.Console.ResetTerminalForegroundColor();

                context.ResultCode = 1;
            }
        }

[jonsequitur]

It's odd that you wouldn't be able to add middleware to a root command using an API exposed on the root command instance.

There's a single middleware pipeline per CommandLineConfiguration, not per Command. The middleware is intended to implement cross-cutting behaviors.

[jnm2]

I think I figured out how to use middleware. Is this right?

return await new CommandLineBuilder(rootCommand)
    .UseDefaults()
    .UseExceptionHandler((exception, context) => context.ResultCode = ExceptionHandler(exception))
    .Build()
    .InvokeAsync(args);

(I missed the call to Build()—updated)

I still would like to use try...catch rather than passing a delegate to an exception handler.

The Symbol types don't know anything about the details of invocation (other than having a pointer to a handler). We wouldn't want to duplicate invocation functionality into Command.

Command owns the state because CommandLineBuilder sets Command.ImplicitParser. I'd rather that the state is exposed transparently wherever it actually lives. If the thing determining how exception handling works is living inside a property inside the root command, wouldn't it make sense to let you access that and configure it?

There's a single middleware pipeline per CommandLineConfiguration, not per Command. The middleware is intended to implement cross-cutting behaviors.

I think I understand this. There's only one root command though, so this could be exposed through RootCommand.

[jnm2]

If it's really imperative that the root command can't have configuration attached to it, what if I could do something like ASP.NET's configuration APIs?

return await rootCommand
    .Configure(config => config.UseDefaultExceptionHandler = false) // Returns some new type or a Parser instance maybe
    .InvokeAsync(args);

Middleware could be added this way too.

return await rootCommand
    .Configure(c => c
        .UseExceptionHandler(false)
        .AddMiddleware(x))
    .InvokeAsync(args);

[jonsequitur]

Command owns the state because CommandLineBuilder sets Command.ImplicitParser.

This is an implementation detail which is subject to change. This is effectively just a cache.

There's only one root command though, so this could be exposed through RootCommand.

The RootCommand isn't involved in invocation of subcommands. It's probably clearer to think of them as orthogonal, independent routes (to use an MVC metaphor.) Just like in ASP.NET, our middleware is intentionally cross-cutting, and no special importance is given to the "root" command other than that its name is derived from the executable's name by default, i.e. it's argv[0]. If you're using the parser in a REPL, for example, you could omit RootCommand from your parser configuration.

[jonsequitur]

return await rootCommand
    .Configure(config => config.UseDefaultExceptionHandler = false) // Returns some new type or a Parser instance maybe
    .InvokeAsync(args);

I think in isolation this API is pretty intuitive. How would it work when someone also does this?

new CommandLineBuilder(rootCommand)
    .UseExceptionHandler()
    .Build();

This why Command.ImplicitParser is really just an implementation detail. The API currently doesn't present users with this confusion.

[jnm2]

Wouldn't it work the same way as the builder already works? Last person to call Build() wins?

new CommandLineBuilder(rootCommand)
    .UseExceptionHandler(x)
    .Build();

new CommandLineBuilder(rootCommand)
    .UseExceptionHandler(y)
    .Build();

[jnm2]

Command.InvokeAsync already effectively performs a new CommandLineBuilder(command)...Build() behind the scenes (even if that's not literally what it does). Something is already happening; I just want a way to pass parameters to configure how that happens, such as UseDefaultExceptionHandler = false.

[jnm2]

Knowing what I know now, I also don't honestly see a conceptual difference between:

var rootCommand = new RootCommand { subcommand1, subcommand2, ... };
rootCommand.Configuration.CrossCuttingStuff(...);
return await rootCommand.InvokeAsync(args);

and

var rootCommand = new RootCommand { subcommand1, subcommand2, ... };

new CommandLineBuilder(rootCommand)
    .CrossCuttingStuff(...);
    .Build();

return await rootCommand.InvokeAsync(args);

Except that the second one seems a lot less obvious to me. If experience hadn't taught me differently, I'd even expect rootCommand.InvokeAsync to not be affected by CommandLineBuilder, and I'd expect to have to call .Build().InvokeAsync(...) in order to make use of the configuration I just did. I'd expect RootCommand.InvokeAsync to be shorthand for creating a brand new builder, calling Build, and calling InvokeAsync on that.

[jnm2]

I apologize for the amount I'm commenting, but I realized that I don't want the ability to use try...catch rather than passing a handler delegate. The reason is that the handler delegate has access to the IConsole and a try...catch around InvokeAsync does not.

[jonsequitur]

Knowing what I know now, I also don't honestly see a conceptual difference between:

I'm not sure there is a meaningful one, but when using CommandLineBuilder, the guidance is to do this, rather than call rootCommand.Invoke:

var rootCommand = new RootCommand { subcommand1, subcommand2, ... };

var parser = new CommandLineBuilder(rootCommand)
    .CrossCuttingStuff(...);
    .Build();

return await parser.InvokeAsync(args);

I think ideally, CommandLineBuilder should not mutate or store state on the RootCommand in any way. It might be a bug that ImplicitParser is set by calling the public Build method.

Currently, a RootCommand doesn't have a 1:1 relationship to a parser, middleware, binding rules, etc. and in fact doesn't know much about them. (Yes, it does have that Handler method, but it's pretty opaque, and ImplicitParser is internal and might be redesigned.) Adding RootCommand.Configuration would add a circular dependency between these layers. Leaving the design decoupled this way is intentional, so that the parser has the potential to ship as a separate component from the invocation and model binding layers, and other approaches to invocation and binding can be added later without having to overhaul the parser.

Some other details to consider:

• You can call, for example, new Command("x").Invoke("some args") or new Option<string>("-x").Parse(). There's no RootCommand in the picture.

• Not all usages of the library use RootCommand (directly, though to some extent they sometimes have to work around it, but we'd like to make these scenarios easier).

[ohadschn]

Any suggestions on an API for opting out of this?

The API could be as simple as UseDefaults(bool handleExceptions) and/or UseDefaults(Action<Exception, InvocationContext> exceptionHandler).

A related problem that needs to be solved is how extension methods like Command.Invoke should behave, given that under the hood they use UseDefaults so that people don't need to use the CommandLineBuilder.

The same (optional) parameters suggested above could be added to the extensions methods and then passed on to their internal UseDefaults call, e.g. Invoke(this Command, string[] args, bool handleExceptions) / Invoke(this Command, string[] args, Action<Exception, InvocationContext> exceptionHandler).

Another option, while we're taking pages off the ASP.NET Core book, could be something like new CommandLineBuilder(rootCommand).UseDefaults().ReplaceExceptionHandler(Action<Exception, InvocationContext> exceptionHandler).

Yet another option would be some static default handler to be used by UseDefaults, e.g. CommandLineBuilder.DefaultExceptionHandler=....

Finally, I think we may have lost sight a little bit of the original desire here - namely easily opt out or replace the default exception handling. It doesn't have to be perfect and cover all possible use cases, just provide those who want to at least one simple and reliable way to do so. For example if I have to create my own CommandLineBuilder and can't use the convenience Command extension methods discussed above, that's not the end of the world.

[AndrewSav]

So the solution I ended up with, in additions to the new CommandLineBuilder(rootCommand).Use multi-line monstrosity is this:

    static class CommandHandlerWrapper
    {
        public static async Task<int> ExecuteCommandHandler<T>(T options, Func<T, Task> handler)
        {
            try
            {
                await handler(options);
                return (int)ExitCode.Success;
            }
            catch (MyAppSpecificException ex)
            {
                return (int)ex.ExitCode;
            }
        }
    }

And then call it as:

command.Handler = CommandHandler.Create<SpecificOptions>(async specificOptions => await 
  CommandHandlerWrapper.ExecuteCommandHandler(specificOptions, CommandClass.Execute));

This avoids the try/catch repetition but introduces the unneeded wrapper,which we can do away with if we implement one of the suggestions above in the command-line-api.

Note that this approach shown is not generic, it requires a certain signature on the likes of CommandClass.Execute and it assumes a single options argument. It also assumes that if the handler does not throw it succeeds (which is correct in my case, but may be incorrect in general).

[SaintGimp]

This is an old issue but it's still open so I'll add my comment: I agree that the current API design is unexpected enough that I felt the need to write a big comment in my own code to explain what's going on:

// Goofy System.CommandLine API going on here.  We need to create and configure
// a builder in order to override its default exception handler and provide our own,
// except that we don't actually use the builder object or its return
// value.  Instead it modifies the root command and then we invoke that.
// https://github.com/dotnet/command-line-api/issues/796
var _ = new CommandLineBuilder(rootCommand)
    .UseDefaults()
    .UseExceptionHandler((e, context) =>
    {
        [custom exception handling here]
    }, 1)
    .Build();

await rootCommand.InvokeAsync(args);

The fact that there's an exception handler inserted by default was frustrating until I figured it out, but better System.CommandLine docs could help avoid that problem in the future. The big API flaw that I'd like to see fixed before an official 2.0 release is that you have to use the CommandLineBuilder to change or remove the exception handler but you don't actually use the builder. It just kind of sits there flapping around. I'd like to see either a .Configure() chain on the RootCommand object instead of the builder, or to be able to run .InvokeAsync() on what CommandLineBuilder.Build() returns. Either one would be self-describing. What we've got now is a head-scratcher.