Program hangs when Environment.Exit is called from OnExecute

[thomaslevesque]

Describe the bug
The program hangs when Environment.Exit(0) is called from OnExecute

To Reproduce
Steps to reproduce the behavior:

1. Using this version of the library: 2.4.2
2. Run this code

    class Program
    {
        static void Main(string[] args)
        {
            CommandLineApplication.Execute<Program>();
        }

        public void OnExecute()
        {
            Console.WriteLine("Hello world");
            Environment.Exit(0);
        }
    }

3. With these arguments: (none)
4. The program hangs

Expected behavior
The program should terminate

Screenshots
N/A

Additional context
I know I don't need to call Environment.Exit, but it's called by the Bullseye library when I use RunTargetsAndExit

[bonesoul]

having the same issue here.

[bonesoul]

downgrading to 2.4.0 fixes issue for me.

[natemcmaster]

Looks like a bug. Would be happy to have help investigating this. I'm low on free time these days

[thomaslevesque]

@natemcmaster I can give it a try. Any idea where I should start looking?

[thomaslevesque]

I see what the problem is. CommandLineUtils subscribe to AppDomain.CurrentDomain.ProcessExit with this handler:

            void processExitHandler(object o, EventArgs e)
            {
                handlerCancellationTokenSource.Cancel();
                handlerCompleted.Wait();
            }

handlerCompleted is a ManualResetEventSlim, which is set when OnExecute completes. But OnExecute can't complete, because it's blocked on the call to Environment.Exit, which won't complete until OnExecute completes... So we have a deadlock.

And as far as I can tell, there's no way to fix it... unless you don't wait for completion in the ProcessExit handler, but I assume this is done for a good reason. Blocking in the ProcessExit handler basically means that any call to Environment.Exit will deadlock.

According to the documentation, in the .NET Framework, the ProcessExit handlers have a 2 seconds timeout, but this timeout doesn't exist in .NET Core. The problem could be mitigated by setting an explicit timeout for handlerCompleted.Wait(). It's not ideal, but at least it wouldn't block indefinitely.

Another option is to make this behavior optional.

@natemcmaster what do you think?

[thomaslevesque]

Apparentlly the problem was introduced in 3a86cab. Previously, handlerCompleted.Wait() was done in the CancelKeyPress handler, which couldn't be triggered by user code.

[thomaslevesque]

Also, I think the handling of Ctrl-C is problematic. Now, Ctrl-C will stop execution only if OnExecute handles the cancellation token. At the very least, Ctrl-C handling should be suppressed if the OnExecute method doesn't accept a cancellation token.

[natemcmaster]

@thomaslevesque thanks for the investigation! This seems patch-worthy to me, so I've created a new branch called 'release/2.4.3'. If you have available time to make a PR, please use that branch. I'm also going to look into making some changes if I have time over the weekend.

The original intent of the Ctrl-C handling was to allow time for async completions after ctrl+c is pressed. I think there are two changes we should make to the current implementation:

1. Only handle ctrl+c if the users calls OnExecuteAsync
2. Remove the ProcessExit event. I'm less convinced now that it's useful to block process exit on async completions. If user code calls Environment.Exit, let's honor that call and let the process exit.

[thomaslevesque]

1. Only handle ctrl+c if the users calls `OnExecuteAsync`

I'm not sure this is the right criteria.

• a sync method can also accept a CancellationToken, so it could also react to ctrl+c.
• a method that doesn't accept a CancellationToken, whether it's sync or async, will not react to ctrl+c, so execution will just continue as if nothing happened.

So I think the only criteria should be that the OnExecute[Async] method accepts a CancellationToken. If it does, let it handle cancellation to stop as soon as it can. If not, leave ctrl+c behavior unchanged ; the app will stop immediately when ctrl+c is pressed.

2. Remove the ProcessExit event. I'm less convinced now that it's useful to block process exit on async completions. If user code calls `Environment.Exit`, let's honor that call and let the process exit.

👍

[bonesoul]

👍 for the removal of processexit event.

[natemcmaster]

So I think the only criteria should be that the OnExecute[Async] method accepts a CancellationToken.

That's what I was originally thinking too, so I'm glad we're on the same page. The implementation of that gets a little tricky with the current code, so let's open a separate issue for this one.

[natemcmaster]

Fixed with #301

Ctrl+c follow up: #302