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 stops working after terminating GUI app once #100

Closed
jstarks opened this issue Nov 5, 2021 · 5 comments
Closed

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

jstarks opened this issue Nov 5, 2021 · 5 comments
Labels
fixed in master Changes have been made but are not in a stable release

Comments

@jstarks
Copy link
Contributor

jstarks commented Nov 5, 2021

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
Copy link
Owner

malxau commented Nov 5, 2021

Thanks for letting me know.

This is going to be related to microsoft/terminal#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:

yori/TODO

Lines 3 to 4 in 1cf1dcc

- Explicit Ctrl+C by detecting console process before launch, if not console
process on same console don't use GenerateConsoleCtrlEvent

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
Copy link
Contributor Author

jstarks commented Nov 5, 2021

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
Copy link
Owner

malxau commented Nov 5, 2021

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
Copy link
Contributor Author

jstarks commented Nov 5, 2021

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
Copy link
Owner

malxau commented Nov 6, 2021

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 0fbf331 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.

@malxau malxau added the fixed in master Changes have been made but are not in a stable release label Nov 6, 2021
@malxau malxau closed this as completed Jan 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed in master Changes have been made but are not in a stable release
Projects
None yet
Development

No branches or pull requests

2 participants