mingw: terminate child processes in a gentler way#2130
Closed
dscho wants to merge 2 commits into
Closed
Conversation
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 <johannes.schindelin@gmx.de>
Previously, we did not install any handler for Ctrl+C, but now we really want to because the MSYS2 runtime learned the trick to call the ConsoleCtrlHandler when Ctrl+C was pressed. With this, hitting Ctrl+C while `git log` is running will only terminate the Git process, but not the pager. This finally matches the behavior on Linux and on macOS. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Member
Author
|
/submit |
|
Submitted as pull.2130.git.1780590261.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This branch is now known as |
|
This patch series was integrated into seen via git@4b8ad25. |
|
This patch series was integrated into next via git@b4a2299. |
|
There was a status update in the "Cooking" section about the branch Advanced emulation of kill() used on Windows in GfW has been upstreamed to improve the symptoms like left-behind .lock files and that fails to let the child clean-up itself when it gets killed. Will merge to 'master'. source: <pull.2130.git.1780590261.gitgitgadget@gmail.com> |
|
There was a status update in the "Cooking" section about the branch Advanced emulation of kill() used on Windows in GfW has been upstreamed to improve the symptoms like left-behind .lock files and that fails to let the child clean-up itself when it gets killed. Will merge to 'master'. source: <pull.2130.git.1780590261.gitgitgadget@gmail.com> |
|
This patch series was integrated into master via git@e444fd1. |
|
Congratulations! 🎉 Your patch series was merged into upstream via e444fd1. Note: this pull request will show as "Closed" rather than "Merged" because the merge happened in the upstream repository, not on GitHub. This is expected — your contribution has been accepted! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This patch series consists of two patches that have been carried in Git for Windows since 2017 an 2018, respectively.
The problem they work around is a fundamental mismatch between Git's understanding how processes can be terminated and Windows' multi-threading centric world view (where multi-process architectures are quite, quite rare), where processes do not tell other processes to terminate gently (meaning: giving them a chance to run their
atexit()handlers).As such, Git thinks that it can send processes signals to terminate or force-stop ("kill") them. There are no signals in the Unix sense on Windows, though. So we try to emulate them. At present, in vanilla Git that means that we use the
TerminateProcess()Win32 API functions, which is most similar to Unix'SIGKILLand is typically frowned upon because it does not allow an orderly shutdown of multi-threaded applications. That's definitely not what Git wants to do: If it wants to terminate a child process, it wants that child process to clean up any.lockfiles, for example. And therefore it wants to send aSIGTERM.But the
SIGTERMsignal does not really have any equivalent on Windows. The closest is to somehow get the target process to call theExitProcess()Win32 API function. There is a trick that we employ here to do precisely that: we create a remote thread in the target process, and specify theExitProcess()function as the callee. This works because that function matches the function signature of thread functions enough that we can get away with it, and because the address of that function is identical between processes matching the same CPU architecture. Read: This approach does not work when trying to terminate i686 processes from an x86_64git.exe. But since it is rare to mix and match processes of different CPU architectures on Windows (certainly in Git scenarios), we kind of resort to this best effort that works often enough to make it worthwhile.It's a different story for
SIGINT: That signal matches most closely what Windows calls a ConsoleCtrlEvent. It is different, though, in that a ConsoleCtrlEvent is not sent to a process, but to a Console, and is handled by all processes that are attached to said Console. In the MSYS2 runtime that provides the POSIX emulation layer required by the Bash distributed with Git for Windows, we work around that by using a similar trick as theSIGTERM/ExitProcess()injection: a thread is injected into the remote process, passing the address of the (undocumented)kernel32!CtrlRoutine. This is quite hacky and requires spawning a separate process to just to figure out the address of said function, which only works in the MSYS2 runtime because it acquires that address once, and then remembers it for the rest of its lifetime. Git also simply has no business emulating a Ctrl+C and instead sends child processesSIGTERM. Therefore, there is no support for sendingSIGINTin this patch series. But patch number 2 implements reacting to the emulatedSIGINT"sent" by the MSYS2 runtime.