Handle Ctrl+C in Git Bash nicely #1170
Conversation
There was a problem hiding this comment.
In general this looks great. I'll assume you've tested it and it works. 😉
I've left a couple of comments. The assumption in kill_child_processes_on_signal that as long as the exit code is greater than 128, it is OK to exit the process might be invalid though - given that GetExitCodeProcess can return on a still running process and an exit code of 259 on Windows means the process is still running.
Approving the changes, as to not be a blocker - but please review the logic in kill_child_processes_on_signal; also please forgive any stupidity I've displayed as I'm still sipping for first cup of coffee ☕️ .
compat/win32/exit-process.h
Outdated
| if (!thread) | ||
| return terminate_process_tree(process, exit_code); | ||
| CloseHandle(thread); | ||
| if (WaitForSingleObject(process, 10000) == WAIT_OBJECT_0) |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| break; | ||
| } | ||
|
|
||
| for (i = len - 1; i >= 0; i--) { |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
compat/mingw.c
Outdated
| * Only continue if the process was terminated by a signal, as | ||
| * indicated by the exit status (128 + sig_no). | ||
| */ | ||
| if (!GetExitCodeProcess(GetCurrentProcess(), &status) || status < 128) |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Yes, I did.
Well, no, the atexit handler is not started unless the process was exited with an exit status ;-) But you are correct, I should be a lot more careful here. I changed the code to look for |
I guess my concern here is that you're asking "what is my exit code" which in theory, because you (the current process) is asking, the value is 259. Which makes the conditional kind of pointless. Does the call to |
compat/mingw.c
Outdated
| * Only continue if the process was terminated by a signal, as | ||
| * indicated by the exit status (128 + sig_no). | ||
| */ | ||
| if (!GetExitCodeProcess(GetCurrentProcess(), &status) || |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
compat/mingw.c
Outdated
| } | ||
|
|
||
| errno = err_win_to_posix(GetLastError()); | ||
| ret = exit_process(h, 3); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
Many of my comments in https://github.com/git-for-windows/msys2-runtime/pull/15/files also apply to this PR. I didn't bother repeating them here. |
Yep. |
|
This revision should be good. |
| if (!initialized) { | ||
| HINSTANCE kernel32 = GetModuleHandle("kernel32"); | ||
| if (!kernel32) | ||
| die("BUG: cannot find kernel32"); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
@dscho please seem my comment on https://github.com/git-for-windows/msys2-runtime/pull/15/files#r117399303 about making this work when msys is 64-bit and the target is 32-bit. |
|
I just had another fresh look over the overall diff, and I realized that the exit code 128 does not signify exit-by-signal (signal numbers start at 1, for |
|
I also realized a problem with the I adjusted the test to verify that |
The TerminateProcess() function does not actually leave the child processes any chance to perform any cleanup operations. This is bad insofar as Git itself expects its signal handlers to run. A symptom is e.g. a left-behind .lock file that would not be left behind if the same operation was run, say, on Linux. To remedy this situation, we use an obscure trick: we inject a thread into the process that needs to be killed and to let that thread run the ExitProcess() function with the desired exit status. Thanks J Wyman for describing this trick. The advantage is that the ExitProcess() function lets the atexit handlers run. While this is still different from what Git expects (i.e. running a signal handler), in practice Git sets up signal handlers and atexit handlers that call the same code to clean up after itself. In case that the gentle method to terminate the process failed, we still fall back to calling TerminateProcess(), but in that case we now also make sure that processes spawned by the spawned process are terminated; TerminateProcess() does not give the spawned process a chance to do so itself. Please note that this change only affects how Git for Windows tries to terminate processes spawned by Git's own executables. Third-party software that *calls* Git and wants to terminate it *still* need to make sure to imitate this gentle method, otherwise this patch will not have any effect. Signed-off-by: Johannes Schindelin <[email protected]>
Git for Windows' MSYS2 runtime was just adjusted to kill processes gently, by injecting a thread that calls ExitProcess(). In case of signals (such as when handling Ctrl+C in a MinTTY window), the exit code is 128 + sign_no, as expected by Git's source code. However, as there is no POSIX signal handling on Windows, no signal handlers are called. Instead, functions registered via atexit() are called. We work around that by testing the exit code explicitly. This fixes the Git for Windows side of the bug where interrupting `git clone https://...` would send the spawned-off `git remote-https` process into the background instead of interrupting it, i.e. the clone would continue and its progress would be reported mercilessly to the console window without the user being able to do anything about it (short of firing up the task manager and killing the appropriate task manually). Signed-off-by: Johannes Schindelin <[email protected]>
When interrupting Git processes in Git Bash by pressing Ctrl+C, [Git now removes `.lock` files as designed](git-for-windows/msys2-runtime#15) ([accompanying Git PR](git-for-windows/git#1170); this should also fix [issue #338](git-for-windows/git#338)). Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely
Handle Ctrl+C in Git Bash nicely
Handle Ctrl+C in Git Bash nicely
Handle Ctrl+C in Git Bash nicely
Handle Ctrl+C in Git Bash nicely
Handle Ctrl+C in Git Bash nicely
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
Handle Ctrl+C in Git Bash nicely Signed-off-by: Johannes Schindelin <[email protected]>
This PR depends on git-for-windows/msys2-runtime#15.
The idea is that the MSYS2 runtime will no longer kill Git's processes using
TerminateProcess(), but by injecting a thread callingExitProcess(), which in turn allows Git'satexit()handlers to run.NOTE: this does not mean that we all of a sudden have POSIX semantics. Far from it: there is still no real signal handling (think
SIGSTOPandSIGCONT), and the signal handlers are not run.Git's source code amazingly seems to be prepared for that case, as most (or all?) signal handlers simply enter code paths that are also entered by atexit handlers.
For example, if a long-running
git cloneis interrupted by Ctrl+C, or by a fatal error, does not matter: in both cases do we want to clean up after us, meaning: the same cleanup functions are called.This is excellent news, as we now can use the same method as the MSYS2 runtime just learned to kill our child processes when we were "exited" by a signal. And those child processes can do the same in return.