When interrupting Win32 processes, kill their child processes, too #6
Conversation
|
This branch needs more testing, style cleanups and a thorough stare under which circumstances After 3h of high focus work on this I need a break, though. |
|
So this is what I did...
$ git clone https://github.com/git-for-windows/git test
Cloning into 'test'...
remote: Counting objects: 209406, done.
remote: Compressing objects: 100% (18/18), done.
Receiving objects: 3% (6283/209406), 2.12 MiB | 813.00 KiB/s
# Ctrl+C
nalla@dev MINGW64 /usr/src
Receiving objects: 13% (27223/209406), 9.41 MiB | 1.37 MiB/s | 1.37 MiB/s
diff --git a/msys2-runtime/PKGBUILD b/msys2-runtime/PKGBUILD
index 1a26c12..43f1100 100644
--- a/msys2-runtime/PKGBUILD
+++ b/msys2-runtime/PKGBUILD
@@ -24,7 +24,7 @@ makedepends=('cocom'
'libiconv-devel'
'diffutils')
# options=('debug' '!strip')
-source=('msys2-runtime'::'git+https://github.com/git-for-windows/msys2-runtime.git#branch=develop')
+source=('msys2-runtime'::'git+https://github.com/dscho/msys2-runtime.git#branch=ctrl-c')
md5sums=('SKIP')
pkgver() {
cd "$srcdir/msys2-runtime"
$ git log --oneline -n2
90fe204 kill: Handle Win32 console processes' children, too
a2cfa19 Kill also child processes of console processes when handling Ctrl+C
C:\git-sdk-64\usr\src\MSYS2-packages\msys2-runtime>C:\git-sdk-64\usr\bin\pacman -U --force msys2-runtime-devel-2.0.16654.90fe204-1-x86_64.pkg.tar.xz msys2-runtime-2.0.16654.90fe204-1-x86_64.pkg.tar.xz
$ pacman -Q msys2-runtime
msys2-runtime 2.0.16654.90fe204-1
|
|
Hmm. I just repeated the procedure (with plain Oh, there is one difference, but that should not really matter... I installed Git via |
|
Taking this from MSDN it seems that |
When a Win32 process needs to be terminated, the child processes it spawned off also need to be terminated. This is no issue for MSys2 processes because MSys2 emulates Unix' signal system accurately, both for the process sending the kill signal and the process receiving it. Win32 processes do not have such a signal handler, though, instead MSys2 shuts them down via `TerminateProcess()`. As `TerminateProcess()` leaves the Win32 process no chance to do anything, and also does not care about child processes, we have to grow a different solution. For console processes, it should be enough to call `GenerateConsoleCtrlEvent()`, but sadly even then this seems to handle child processes correctly only on Windows 8 but not Windows 7. So let's identify the tree of processes spawned directly and indirectly from the process to be killed, and kill them all. To do so, we do not use the NtQueryInformationProcess() function because 1) it is marked internal and subject to change at any time of Microsoft's choosing, and 2) it does not even officially report the child/parent relationship (the pid of the parent process is stored in the `Reserved3` slot of the `PROCESS_BASIC_INFORMATION` structure). Instead, we resort to the Toolhelp32 API -- which happily also works on 64-bit Windows -- to enumerate the process tree and reconstruct the process tree rooted in the process we intend to kill. This fixes 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). Note that this special-handling is only necessary when *MSys* handles the Ctrl+C event, e.g. when interrupting a process started from within mintty or any other non-cmd-based terminal emulator. If the process was started from within `cmd.exe`'s terminal window, child processes are already killed appropriately upon Ctrl+C. Signed-off-by: Johannes Schindelin <[email protected]>
... for use in kill.exe. Signed-off-by: Johannes Schindelin <[email protected]>
This change is the equivalent to the change to the Ctrl+C handling we just made. Signed-off-by: Johannes Schindelin <[email protected]>
dscho
changed the title
|
I also just changed the first description to spare the occasional reader having to read obsolete (and incorrect) information. IMO this PR is in a pretty good shape now and ready for review. @t-b @sschuberth would you give it a look, please? |
|
I don't have the time to review the implementation (and I guess either has @t-b), but the intention sounds good to me, so I'll merge as I trust @dscho to have tested the implementation thoroughly. That said, I still think we should not focus too much on getting mingw-git to run with mintty, and use an MSYS2-independent console program instead. |
When interrupting Win32 processes, kill their child processes, too
@sschuberth It does not matter whether we end up with mintty or any other terminal emulator. As long as we get away from the By the way, I have no idea why you are opposed to mintty. It is small (~170kB) and does everything we need. Would you care to clarify your reservations, please? |
The only single reason, as already stated several times, is this issue of mintty. I like mintty itself very much for its looks and compactness, but very sadly it's simply of no use for running native Windows applications. |
|
The issue you mention affects only native Windows applications that try to interact natively with the console. Sadly, this issue will be shared with all terminal emulators that are not based on the cmd terminal window. The only one I can think of is |
|
Because Console2 (or better ConsoleZ) is not the only terminal, there's also the already mentioned ConEmu, both of which can be combined with clink. Cmder already bundles ConEmu with clink into a portable application. I like each of these better than mintty because they are more generic in that they can be used with any command line / script interpreter, be it Bash, cmd or PowerShell. |
|
Okay, fine. For the moment, I will continue on the basis on mintty, though. It is not at all clear if any of the alternatives you listed would fare better when trying to call |
It is clear, in the sense that it is documented. All native console programs run inside ConEmu / ConsoleZ behave exactly the same in terms of Note that it is a common misconception that |
|
Okay. Still skeptical about those limitations, but I'll leave the task of picking a better terminal to you, then. I am getting really, really close now to having a portable application (sadly, it seems that the size of the archive will jump from ~24MB to ~31MB, but that is much better than I feared). I would even vote for going for mintty for the first portable release of GIt for Windows 2.x, just because it works well enough for yours truly now. |
|
I vote for |
|
Don't get me wrong. As I said, I like mintty. I just though we could save the work getting it to run with mingw-git by using one of the mentioned console apps. Now as @dscho as already done much of that work, it's probably fine to ship with mintty. But if we do that, we should be prepared for bug reports in case there are remaining issues with it, and we should be willing to fix them. |
Note that this is my preferred choice for the portable Git for Windows, because mintty uses up only ~170kB in addition to msys2-runtime (which does the heavy-lifting). |
I am. Note, also, that "I cannot run an interactive Python session in Git for Windows" is not really a valid bug report for Git for Windows. All Git for Windows needs to provide is a way to run Git correctly. And that we can do so far. |
Cmder also (optionally) bundles git. After a MSYS2-based Git for Windows gets released (awesome job so far, very glad to see this happening), they will surely update their bundled version, so anyone who wants cmder can easily get latest git alongside it. ConEmu-based emulators can cause serious performance problems with process spawning, see e.g. JuliaLang/julia#7267 (comment), and cmderdev/cmder#188 may be closely related. |
|
@jan-hudec I know that this ticket has a really messed up history. It discusses almost anything but what the ticket is actually about. Let's correct that mistake and stay on topic. If you want to discuss another issue, open a ticket for it, please. |
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
When interrupting Win32 processes, kill their child processes, too
The symptom fixed by this PR is that interrupting a
git clone https://...with Ctrl+C in mintty does not terminate the clone, but sends the process into the background, with an unnerving unstoppable progress output to the terminal.The reason is that calling
TerminateProcess()does not affect the processes spawned off of the terminated process.Note: this only affects processes interrupted in
minttyvia Ctrl+C; If the Ctrl+C is handled in acmd.exe-backed terminal window, it is already handled correctly.