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.

Sign up for GitHub

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

Jump to bottom

Ctrl+C revisited #15

Merged

dscho merged 3 commits into git-for-windows:master from dscho:dscho/ctrl-c-fixup

May 19, 2017

Member

dscho commented May 18, 2017

@whoisj was kind enough to point me to an excellent way to terminate processes in a gentler way than the current method: instead of calling TerminateProcess() (which gives the process no chance of some last-second cleanup), we now inject a thread that calls ExitProcess().

While at it, we also change the exit code to 128 + signal when terminating the process via signal, as POSIX folks would expect it.

This is the MSYS2 side of my effort to fix stale .lock files upon interrupting Git processes.


          fixup! Export the kill_process_tree() function

82014d1

This drops the patch. After making up my mind about it, it would appear to
be a bad idea to export this via the .dll. Better implement the functions
in a header file that needs to be #include'd by the callers, and marks the
functions as file-local.

Signed-off-by: Johannes Schindelin <[email protected]>

dscho requested a review from whoisj

May 18, 2017 13:38

Member Author

dscho commented May 18, 2017

This should fix git-for-windows/git#338, and may fix git-for-windows/git#1134 and git-for-windows/git#703.

dscho mentioned this pull request

Handle Ctrl+C in Git Bash nicely git-for-windows/git#1170

Merged

jeffhostetler reviewed

View reviewed changes

winsup/utils/kill.cc Outdated

+                HANDLE h = OpenProcess (PROCESS_CREATE_THREAD | PROCESS_QUERY_INFORMATION |
+              			  PROCESS_VM_OPERATION | PROCESS_VM_WRITE |
+              			  PROCESS_VM_READ | PROCESS_TERMINATE,
+              			  FALSE, (DWORD) dwpid);
                 if (!h)

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

jeffhostetler reviewed

View reviewed changes

winsup/cygwin/include/cygwin/version.h

                 Note that we forgot to bump the api for ualarm, strtoll, strtoull,
                 sigaltstack, sethostname. */
               #define CYGWIN_VERSION_API_MAJOR 0
-              #define CYGWIN_VERSION_API_MINOR 310
+              #define CYGWIN_VERSION_API_MINOR 309

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

jeffhostetler reviewed

View reviewed changes

winsup/cygwin/include/cygwin/exit_process.h Outdated

+                for (i = len - 1; i >= 0; i--)
+                  {
+                    HANDLE process = i == 0 ? main_process :
+                      OpenProcess(PROCESS_TERMINATE, FALSE, pids[i]);

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

jeffhostetler reviewed

View reviewed changes

jeffhostetler left a comment

I made some notes on this.

whoisj approved these changes

View reviewed changes

whoisj left a comment

Lgtm. Left a few comments, but clicking approve so as to not block (I know, nobody can really block @dscho from commiting - but you get my point).

winsup/cygwin/include/cygwin/exit_process.h Outdated

+                                                        (PVOID)exit_code, 0, &thread_id);
+                    if (!thread)
+                      return terminate_process_tree(process, exit_code);

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

winsup/utils/kill.cc Outdated

+                HANDLE h = OpenProcess (PROCESS_CREATE_THREAD | PROCESS_QUERY_INFORMATION |
+              			  PROCESS_VM_OPERATION | PROCESS_VM_WRITE |
+              			  PROCESS_VM_READ | PROCESS_TERMINATE,
+              			  FALSE, (DWORD) dwpid);
                 if (!h)

This comment was marked as off-topic.

Sign in to view

winsup/cygwin/exceptions.cc

-                      kill_process_tree (GetProcessId (ch_spawn), sigExeced = si.si_signo);
-                    else
-                      TerminateProcess (ch_spawn, sigExeced = si.si_signo);
+                    exit_process (ch_spawn, 128 + (sigExeced = si.si_signo));

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

dscho force-pushed the dscho/ctrl-c-fixup branch from 664e634 to 2605c89 Compare

May 18, 2017 18:29

jeffhostetler reviewed

View reviewed changes

winsup/cygwin/include/cygwin/exit_process.h Outdated

+                      {
+                        if (!TerminateProcess(process, exit_code))
+                          ret = -1;
+                        CloseHandle(process);

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

jeffhostetler reviewed

View reviewed changes

winsup/cygwin/include/cygwin/exit_process.h

+               * Inject a thread into the given process that runs ExitProcess().
+               *
+               * Note: as kernel32.dll is loaded before any process, the other process and
+               * this process will have ExitProcess() at the same address.

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

jeffhostetler reviewed

View reviewed changes

winsup/cygwin/include/cygwin/exit_process.h Outdated

+                if (GetExitCodeProcess (process, &code) && code == STILL_ACTIVE)
+                  {
+                    HINSTANCE kernel32 = GetModuleHandle("kernel32");

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

jeffhostetler reviewed

View reviewed changes

winsup/utils/kill.cc

@@ @@ -173,7 +174,17 @@ forcekill (int pid, int sig, int wait) @@
                 if (!wait || WaitForSingleObject (h, 200) != WAIT_OBJECT_0)
                   {
                     if (sig == SIGINT || sig == SIGTERM)
-                      kill_process_tree (dwpid, sig);
+                      {

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

jeffhostetler reviewed

View reviewed changes

winsup/cygwin/include/cygwin/exit_process.h

+               * This function expects the process handle to have the access rights for
+               * CreateRemoteThread(): PROCESS_CREATE_THREAD, PROCESS_QUERY_INFORMATION,
+               * PROCESS_VM_OPERATION, PROCESS_VM_WRITE, and PROCESS_VM_READ.
+               *

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

jeffhostetler reviewed

View reviewed changes

winsup/cygwin/include/cygwin/exit_process.h Outdated

+                    HINSTANCE kernel32 = GetModuleHandle("kernel32");
+                    LPTHREAD_START_ROUTINE exit_process_address =
+                      (LPTHREAD_START_ROUTINE) GetProcAddress (kernel32, "ExitProcess");
+                    DWORD thread_id;

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

jeffhostetler reviewed

View reviewed changes

jeffhostetler left a comment

I've reviewed version 2. Let me know if you have questions.

dscho force-pushed the dscho/ctrl-c-fixup branch 3 times, most recently from 3f1bb83 to 6888abe Compare

May 18, 2017 20:49

Member Author

dscho commented May 18, 2017

Okay, this revision should be good.

dscho added 2 commits

May 19, 2017 12:01


          squash! Kill also children of the process to be killed via Ctrl+C

e9cb332

Try to kill Win32 processes gently upon Ctrl+C

When a process is terminated via TerminateProcess(), it has no chance to
do anything in the way of cleaning up. This is particularly noticeable
when a lengthy Git for Windows process tries to update Git's index file
and leaves behind an index.lock file. Git's idea is to remove the stale
index.lock file in that case, using the signal and atexit handlers
available in Linux. But those signal handlers never run.

Note: this is not an 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()`.

Let's use a gentler method, described in the Dr Dobb's article "A Safer
Alternative to TerminateProcess()" by Andrew Tucker (July 1, 1999),
http://www.drdobbs.com/a-safer-alternative-to-terminateprocess/184416547

Essentially, we inject a new thread into the running process that does
nothing else than running the ExitProcess() function.

If that fails, or when the process still lives on after 10 seconds, we
fall back to calling TerminateProcess(), but in that case we take pains
to kill also processes directly or indirectly spawned by that process;
TerminateProcess() does not give any process a chance to terminate its
child processes. Originally, we wanted to call the function
`GenerateConsoleCtrlEvent()` for SIGTERM with console processes, but 1)
that may not work as it requires a process *group* ID (i.e. the process
ID of the initial process group member, which we may not have), and 2)
it does not kill the process *tree* on Windows 7 and earlier.

Note: 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.

While at it, we also fix the exit status: the convention is to add 128
to the signal number, to give exit code handlers a chance to detect an
"exit by signal" condition.

This fixes the MSYS2 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).

Note that this special-handling is only necessary when *MSYS2* 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]>


          squash! kill: Handle Win32 console processes' children, too

f4a52b9

kill: kill Win32 processes more gently

This change is the equivalent to the change to the Ctrl+C handling we
just made.

Signed-off-by: Johannes Schindelin <[email protected]>

dscho force-pushed the dscho/ctrl-c-fixup branch from 6888abe to f4a52b9 Compare

May 19, 2017 10:02

Member Author

dscho commented May 19, 2017

Some last-minute touch-ups: the test whether the target process has the same architecture as the current process was fixed (remember, the calling process can be a WoW process itself), some code style adjustments and a few more comments.

dscho merged commit daa03e3 into git-for-windows:master

dscho added a commit to git-for-windows/MSYS2-packages that referenced this pull request


          msys2-runtime: update to include the Ctrl+C fixes

1bac81b

This revamps the way Git for Windows' fork of the MSYS2 runtime kills
processes: it is done much gentler now, giving the target processes a
chance to clean up after themselves in their atexit handlers.

The fixes were merged via
git-for-windows/msys2-runtime#15

Signed-off-by: Johannes Schindelin <[email protected]>

dscho deleted the dscho/ctrl-c-fixup branch

May 19, 2017 10:53

dscho added a commit to git-for-windows/build-extra that referenced this pull request


          Mention Bug Fix in release notes

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]>

dscho mentioned this pull request

Bug: Cannot kill Nodejs process using ctrl + c git-for-windows/git#1219

Closed

1 task

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet