Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ctrl+C doesn't kill the process #302

Closed
natemcmaster opened this issue Nov 1, 2019 · 5 comments
Closed

Ctrl+C doesn't kill the process #302

natemcmaster opened this issue Nov 1, 2019 · 5 comments
Assignees
Labels
Milestone

Comments

@natemcmaster
Copy link
Owner

Follow-up to #292

Regression in 3a86cab

Repro:

using System;
using System.Threading;
using McMaster.Extensions.CommandLineUtils;

namespace repro
{
    class Program
    {
        static int Main(string[] args)
        {
            return CommandLineApplication.Execute<Program>(args);
        }

        private void OnExecute()
        {
            Console.CancelKeyPress += (_, __) =>
            {
                Console.WriteLine("CTRL+C pressed");
            };
            Console.WriteLine("Hello World!");
            Thread.Sleep(10000);
            Console.WriteLine("Bye world!");
        }
    }
}

Result:

$ dotnet run
Hello World!
^CCTRL+C pressed
Bye world!
@thomaslevesque
Copy link
Contributor

From #292:

The implementation of that gets a little tricky with the current code

That was my impression too. Currently the OnExecute[Async] method is resolved "just in time", so we can't know in advance whether it accepts a CancellationToken.

Here's how I would approach it:

  • Add an internal SupportsCancellation property to CommandLineApplication
  • In ExecuteMethodConvention, resolve the execute method eagerly, and set the SupportsCancellation property on the CommandLineApplication
  • in CommandLineApplication.ExecuteAsync, subscribe to CancelKeyPress only if SupportsCancellation is true

@natemcmaster if you agree I can submit a PR, unless you prefer to do it yourself

@natemcmaster
Copy link
Owner Author

Yes, this seems like the right approach. A PR would be greatly appreciated!

@natemcmaster natemcmaster added the help wanted We would be willing to take a well-written PR to help fix this. label Nov 5, 2019
@iprins
Copy link

iprins commented Nov 11, 2019

I'm running into the same issue, when will the PR be merged?

natemcmaster added a commit that referenced this issue Nov 11, 2019
@natemcmaster
Copy link
Owner Author

@thomaslevesque - I reviewed your PR and it matches what you proposed above, but I don't think I'm going to merge it yet. I've been thinking about this all week and think that Ctrl+C needs to be revisited altogether. I'm concerned users won't understand that the ctrl+c behavior changes based on which overload of OnExecute they invoke.

I'm preparing a 2.4.4 patch which simply removes the blocking on async completions behavior. 5f724a7 If blocking on async completions is something you still want, let's open a new issue and look into adding an opt-in API for this behavior.

For anyone who had come to depend on the behavior of waiting for async completions to finalize before process exit, they can restore the original behavior in 2.4.4 by adding code like this before CommandLineApplication runs.

Console.CancelKeyPress += (o, e) =>
{
   e.Cancel = true;
};

CancellationToken will still be cancelled, but default ctrl+c handling isn't going to wait.

@natemcmaster natemcmaster removed the help wanted We would be willing to take a well-written PR to help fix this. label Nov 11, 2019
@thomaslevesque
Copy link
Contributor

Sound like a reasonable approach 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants