natemcmaster/CommandLineUtils

OperationCanceledException in ExecuteAsync is always masked as caused by SIGINT

lGSMl opened this issue · 1 comments

lGSMl commented

Describe the bug
ExecuteAsync implementation in CommandLineApplication (from 3.1.0-beta, but it was not changed much in 3.1.0):

public async Task<int> ExecuteAsync(string[] args, CancellationToken cancellationToken = default)
#pragma warning restore RS0026 // Do not add multiple public overloads with optional parameters
        {
            var parseResult = Parse(args);
            var command = parseResult.SelectedCommand;

            if (command.IsShowingInformation)
            {
                return HelpExitCode;
            }

            var validationResult = command.GetValidationResult();
            if (validationResult != ValidationResult.Success)
            {
                return command.ValidationErrorHandler(validationResult);
            }

            var handlerCancellationTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);

            void cancelHandler(object o, ConsoleCancelEventArgs e)
            {
                handlerCancellationTokenSource.Cancel();
            }

            try
            {
                _context.Console.CancelKeyPress += cancelHandler;

                return await command._handler(handlerCancellationTokenSource.Token);
            }
            catch (OperationCanceledException)
            {
                return ExitCodeOperationCanceled;
            }
            finally
            {
                _context.Console.CancelKeyPress -= cancelHandler;
            }
        }

In try/catch block it is implied that any OperationCanceledException is caused by user Ctrl-C input, while it can be caused by any other completely unrelated reason in further callstack.

In my scenario I get confusing sigint exit codes when underlying http requests time out - System.Net.Http just throws OperationCanceledException on request timeout.

To Reproduce
Throw OperationCanceledException inside the _handler delegate.

Expected behavior
Exception is rethrown to caller, and intercepted only if cancellation reason was actual console sigint event.

Seems reasonable to me. My intention in adding catch (OperationCanceledException) was for users that invoke the static CommandLineApplication.Execute methods. We can move the catch handler up the stack into those methods instead.