vezel-dev / cathode

A terminal-centric replacement for the .NET console APIs.
https://docs.vezel.dev/cathode
BSD Zero Clause License
91 stars 7 forks source link

Debugging a readline app and CTRL+C makes the app stuck in a loop #64

Closed xoofx closed 2 years ago

xoofx commented 2 years ago

If you debug with Visual Studio the following program (with a CMD opened on Windows), and press CTRL+C, it won't exit:

Terminal.OutLine("Type some text:");
while (true)
{
    var line = Terminal.ReadLine();
    if (line == null) break;
    Terminal.OutLine(line);
}

It seems to be stuck in the following loop (it catches the CTRL+C but ignore it and continue)

https://github.com/alexrp/system-terminal/blob/adf2a455ea0dbc35f48d3204179324a7e37c0d82/src/core/Terminals/Windows/WindowsTerminalReader.cs#L57-L62

Running the app without debugging will properly exit the app.

Terminal: Compiled from latest master adf2a45

alexrp commented 2 years ago

This is probably because we set up a SIGINT (CTRL_C_EVENT) handler at startup.

Can you check if removing the handler here fixes the issue?

https://github.com/alexrp/system-terminal/blob/adf2a455ea0dbc35f48d3204179324a7e37c0d82/src/core/SystemVirtualTerminal.cs#L127

xoofx commented 2 years ago

Can you check if removing the handler here fixes the issue?

It doesn't but If I remove all handlers, it passes. I tried also to remove some handlers but it didn't help.

alexrp commented 2 years ago

but If I remove all handlers, it passes

Makes sense:

https://github.com/dotnet/runtime/blob/af9202a6daf91ba58d5d58bafbb6581aebb8a642/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Windows.cs#L13-L60

I suppose we could create the registrations lazily when an event handler is added so that things work as expected in most cases. But if an event handler is added at any point or an app by some other means causes a PosixSignalRegistration to be created, this behavior would occur again. That might be acceptable though.

xoofx commented 2 years ago

I suppose we could create the registrations lazily when an event handler is added so that things work as expected in most cases. But if an event handler is added at any point or an app by some other means causes a PosixSignalRegistration to be created, this behavior would occur again. That might be acceptable though.

I don't know but it is quite annoying to have such behavior. It makes PosixSignalRegistration almost unusable on Windows so I would believe that it would have to be implemented differently on Windows (e.g propagate the CTRL+C from the ReadBufferCore after ReadConsoleW instead of using PosixSignalRegistration - awfully ugly I agree 😅) This whole thing of trying to replicate the behavior of posix signals in .NET seems brittle in the presence of a non-POSIX system as it can conflict with other behaviors in place...

xoofx commented 2 years ago

I have hacked something like that at the end of the HandleSignal and it gives a similar behavior to running without the debugger attached:

if (Debugger.IsAttached && !ctx.Cancel && context.Signal == PosixSignal.SIGINT)
{
    Environment.Exit(-1073741510);
}

Does it sound too hacky to add it?

xoofx commented 2 years ago

Pushed a PR to let you see what are the changes to workaround this.

alexrp commented 2 years ago

Hmm, it's admittedly been a while since I debugged anything in Visual Studio, but if my memory serves, pressing Ctrl-C in a console window while it's being debugged should be roughly equivalent to a debug break, no? Is exiting actually the expected behavior?

xoofx commented 2 years ago

pressing Ctrl-C in a console window while it's being debugged should be roughly equivalent to a debug break, no? Is exiting actually the expected behavior?

D'oh! Actually I checked with a normal Console app or even without using Console at all and CTRL-C is actually sending a native exception to the debugger (though, even with the right settings, didn't get in the debugger, only in the logs)... so It seems that this behavior has been around for a while... completely unrelated to Terminal, sorry for the noise!

alexrp commented 2 years ago

I think there's still an improvement to be made here though. We should only create those 4 PosixSignalRegistration instances if/when someone actually adds a Signaled event handler. That way we at least match the default .NET Console behavior. I'll have a look at that.

alexrp commented 2 years ago

We should only create those 4 PosixSignalRegistration instances if/when someone actually adds a Signaled event handler.

Done in 29c42665627efac8c31fb94b940d8c0a68080fa3.