command-line-api: How do we implement exception handling around Command.Invoke?

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.

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…

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?

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.

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.

@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.

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.

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.

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

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.

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.

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.

A Criticism:

I generally do not give negative feedback since I highly value the time and effort smart people put into developing open-source tools and making them freely available to everyone. This library has some great design decisions and is hugely necessary for anyone developing console apps. I have spent over two weeks full-time migrating from Microsoft.Extensions.CommandLineUtils to this library, a ~300 lines of code in that library is now almost 1000 lines in this library. The binding context is the biggest mess. However, I’m going to revert all the changes back to Microsoft.Extensions.CommandLineUtils since this library adds more problems than those it solves; particularly:

• Your decision around catching any exception and building a middleware is something I do not see very often, and that is the single point of failure. This thread is two years long and you have not yet provided a good solution. I barely get the exit code correctly in the following code. For instance, how would you handle exceptions in BindingContext?

var parser = new CommandLineBuilder(rootCmd)
     .UseExceptionHandler((e, context) =>
     {
        exceptionHandler(e, context);
     }, errorExitCode: 1)

• I have cases where the parser.Invoke does not return after the exception is handled in the above code; so the ExitCode does not propagate.

• It seems when NotImplementedException is thrown in the command handler, the program just exits after calling exceptionHandler, while I expect Invoke to return the given exit code instead.

@jonsequitur Aha, I see what’s leading people (well, me at least) down the wrong path and into the ditch. The Parse class doesn’t have an InvokeAsync method. That’s actually an extension method and you have to have using System.CommandLine.Parsing; in order to get it. If you aren’t already using the System.CommandLine.Parsing namespace, then you’ll think that parser.InvokeAsync() doesn’t exist, but RootCommand.InvokeAsync() does, and you’ll discover that the code I wrote above does actually work (though it looks goofy), and you’ll end up thinking that’s the only way to do things.

I saw docs and code samples using Parser.InvokeAsync but when I couldn’t get it to compile I assumed that it had been removed with all of the other API changes rather than investigating whether I needed another namespace.

I don’t know if the InvokeAsync methods are extension methods for some really good reason, but it seems that if the fixed code you offered above is the normal pattern that people are intended to use, it should probably work without having to play “hunt the namespace”. 😊

It might make sense to move those extension methods into the System.CommandLine namespace so they’ll be more discoverable.

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.

Oh yeah, I found that funny too. Not major, but a code smell. I agree with all you just said. For an opportunity to create a brand new API those choices seemed strange. jonsequitur tried to justify them above but I’m not convinced. Having said that I have not followed what changed in the library in this respect in the last year, but judging by your comment not much.

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.

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.