search

malxau / yori

Yori is a CMD replacement shell that supports backquotes, job control, and improves tab completion, file matching, aliases, command history, and more.
http://www.malsmith.net/yori/
MIT License
1.24k stars 31 forks source link

Ctrl-C stops working after terminating GUI app once #100

Closed jstarks closed 2 years ago

jstarks commented 2 years ago

Yori 1.60.

Repro steps:

> notepad.exe
   --- *ctrl-c*, notepad terminates
> notepad.exe
   --- *ctrl-c*, nothing happens
> ping microsoft.com
   --- *ctrl-c*, nothing happens

I haven't debugged this to see what's going on yet.

malxau commented 2 years ago

Thanks for letting me know.

This is going to be related to https://github.com/microsoft/terminal/issues/335 . I thought I had worked around this sufficiently, but frankly, it's really hard to work around. It turns the whole feature into a minefield.

This is also the second item in TODO: https://github.com/malxau/yori/blob/1cf1dcce00d026c01019f142fb1ae23e519afb7b/TODO#L3-L4

I haven't touched it because I expected it to be disruptive, particularly in terms of performance. It needs to inspect the process on launch and distinguish GUI from console processes, as well as processes which are intentionally started on a different console. Based on the process type, either use GenerateConsoleCtrlEvent when safe (console process on this console) or fall back to TerminateProcess when not.

The way to do it with documented APIs is to always CREATE_SUSPENDED, inspect the executable header, then let the process go, which seems like a slow way to start a process. But (a couple years later), I think this is what CreateProcess is doing anyway, so moving the resume up a layer isn't going to slow process creation down meaningfully, so I should just start down this road and see where it goes.

But yeah, the Terminal team's "mass chaos" label applies. And real kudos to eryksun for figuring it out without source code.

jstarks commented 2 years ago

Fascinating.

IIRC, cmd.exe opens the .exe and inspects the header before even calling CreateProcess, since it needs to distinguish between console and GUI apps to determine whether to wait. Unclear whether that's any slower than the CREATE_SUSPENDED idea.

(cmd's GUI non-wait behavior is actually my preference for now, since Yori doesn't yet support Ctrl-Z yet. It's annoying to launch a GUI app and have no recourse but to terminate it to get back control of the console. Unless I missed a feature.)

malxau commented 2 years ago

Ctrl+B. This aborts the wait and leaves the child running.

Ctrl+Z (abort shell wait and suspend child) is something I'd like to implement, but the vast majority of times I'm using it, it's followed immediately by bg (the goal was to get the prompt back, not suspend the process.)

Suspend on NT is a bit of a mess since the only documented way to do it is with the debugger APIs, and those were incomplete in early versions. It's possible to attach (thereby suspending the process), but not detach, and suspending the process for a second time requires getting the child to issue a DebugBreak. In early versions, that means allocating memory in the child and call CreateRemoteThread. That should probably happen while the process is suspended, meaning the debugger injects a thread that can use a shared event or similar to determine when to break into the debugger, so the debugger can ask the child to break in response to shell commands. Cross-architecture debugging is always a bit odd; I think it's not possible for a 32 bit shell to do this for a 64 bit child, and a 64 bit shell would need to inject the right code depending on the architecture of the child. Arguably the debug attach behavior would be for everything in the process tree too. Since suspend isn't something I really used much, it didn't seem worth the undertaking to make it really work.

jstarks commented 2 years ago

Cool, didn't know about Ctrl-B, thanks. I also usually just bg right away so that's pretty reasonable.

Regarding suspend, surely NtSuspendProcess isn't a big secret at this point.

malxau commented 2 years ago

For NtSuspendProcess, the issue isn't that it's a secret, the issue is it was only exported on XP and Yori runs on 3.1. XP has a few changes to make the debugging interfaces much more sane too.

For the original issue, I just pushed commit 0fbf3316542fe7bac2781a2c890d1f1421990ec1 for this and it looks more straightforward than I was expecting. Now I'm wondering if it makes sense to have an option for Yori to mimic CMD's behavior with respect to not waiting for GUI processes, since the detection part is done; personally I've grown attached to its current behavior, but I could mimic CMD if desired.