Win, #519: FIX WinHangs: Popen() CREATE_NEW_PROCESS_GROUP to allow kill

+ FIXED most hangs BUT no more `git-daemon` un-killable!
+ Use logger for utils to replace stray print().

4 files changed, 26 insertions, 17 deletions

diff --git a/git/cmd.py b/git/cmd.py
index 14f655ed..f6cb0ce9 100644
--- a/git/cmd.py
+++ b/git/cmd.py
@@ -15,6 +15,7 @@ import mmap
 from git.odict import OrderedDict
 from contextlib import contextmanager
 import signal
+import subprocess
 from subprocess import (
     call,
     Popen,
@@ -229,6 +230,15 @@ def dict_to_slots_and__excluded_are_none(self, d, excluded=()):
 
 ## -- End Utilities -- @}
 
+# value of Windows process creation flag taken from MSDN
+CREATE_NO_WINDOW = 0x08000000
+
+## CREATE_NEW_PROCESS_GROUP is needed to allow killing it afterwards,
+# seehttps://docs.python.org/3/library/subprocess.html#subprocess.Popen.send_signal
+PROC_CREATIONFLAGS = (CREATE_NO_WINDOW | subprocess.CREATE_NEW_PROCESS_GROUP
+                      if sys.platform == 'win32'
+                      else 0)
+
 
 class Git(LazyMixin):
 
@@ -267,9 +277,6 @@ class Git(LazyMixin):
     # Enables debugging of GitPython's git commands
     GIT_PYTHON_TRACE = os.environ.get("GIT_PYTHON_TRACE", False)
 
-    # value of Windows process creation flag taken from MSDN
-    CREATE_NO_WINDOW = 0x08000000
-
     # Provide the full path to the git executable. Otherwise it assumes git is in the path
     _git_exec_env_var = "GIT_PYTHON_GIT_EXECUTABLE"
     GIT_PYTHON_GIT_EXECUTABLE = os.environ.get(_git_exec_env_var, git_exec_name)
@@ -317,7 +324,7 @@ class Git(LazyMixin):
 
             # try to kill it
             try:
-                os.kill(proc.pid, 2)   # interrupt signal
+                proc.terminate()
                 proc.wait()    # ensure process goes away
             except (OSError, WindowsError):
                 pass  # ignore error when process already died
@@ -632,7 +639,6 @@ class Git(LazyMixin):
                 cmd_not_found_exception = OSError
         # end handle
 
-        creationflags = self.CREATE_NO_WINDOW if sys.platform == 'win32' else 0
         try:
             proc = Popen(command,
                          env=env,
@@ -644,7 +650,7 @@ class Git(LazyMixin):
                          shell=self.USE_SHELL,
                          close_fds=(os.name == 'posix'),  # unsupported on windows
                          universal_newlines=universal_newlines,
-                         creationflags=creationflags,
+                         creationflags=PROC_CREATIONFLAGS,
                          **subprocess_kwargs
                          )
         except cmd_not_found_exception as err:
@@ -655,7 +661,8 @@ class Git(LazyMixin):
 
         def _kill_process(pid):
             """ Callback method to kill a process. """
-            p = Popen(['ps', '--ppid', str(pid)], stdout=PIPE, creationflags=creationflags)
+            p = Popen(['ps', '--ppid', str(pid)], stdout=PIPE,
+                      creationflags=PROC_CREATIONFLAGS)
             child_pids = []
             for line in p.stdout:
                 if len(line.split()) > 0:
diff --git a/git/index/fun.py b/git/index/fun.py
index 6026e232..818847a2 100644
--- a/git/index/fun.py
+++ b/git/index/fun.py
@@ -12,11 +12,10 @@ from stat import (
 
 from io import BytesIO
 import os
-import sys
 import subprocess
 
 from git.util import IndexFileSHA1Writer
-from git.cmd import Git
+from git.cmd import PROC_CREATIONFLAGS
 from git.exc import (
     UnmergedEntriesError,
     HookExecutionError
@@ -78,7 +77,7 @@ def run_commit_hook(name, index):
                            cwd=index.repo.working_dir,
                            close_fds=(os.name == 'posix'),
                            universal_newlines=True,
-                           creationflags=Git.CREATE_NO_WINDOW if sys.platform == 'win32' else 0,)
+                           creationflags=PROC_CREATIONFLAGS,)
     stdout, stderr = cmd.communicate()
     cmd.stdout.close()
     cmd.stderr.close()
diff --git a/git/test/lib/helper.py b/git/test/lib/helper.py
index 9488005f..b59f518b 100644
--- a/git/test/lib/helper.py
+++ b/git/test/lib/helper.py
@@ -5,12 +5,12 @@
 # the BSD License: http://www.opensource.org/licenses/bsd-license.php
 from __future__ import print_function
 import os
-import sys
 from unittest import TestCase
 import time
 import tempfile
 import shutil
 import io
+import logging
 
 from git import Repo, Remote, GitCommandError, Git
 from git.compat import string_types
@@ -25,6 +25,8 @@ __all__ = (
     'with_rw_repo', 'with_rw_and_rw_remote_repo', 'TestBase', 'TestCase', 'GIT_REPO', 'GIT_DAEMON_PORT'
 )
 
+log = logging.getLogger('git.util')
+
 #{ Routines
 
 
@@ -120,7 +122,7 @@ def with_rw_repo(working_tree_ref, bare=False):
                 try:
                     return func(self, rw_repo)
                 except:
-                    print("Keeping repo after failure: %s" % repo_dir, file=sys.stderr)
+                    log.info("Keeping repo after failure: %s", repo_dir)
                     repo_dir = None
                     raise
             finally:
@@ -218,7 +220,7 @@ def with_rw_and_rw_remote_repo(working_tree_ref):
                 # on some platforms ?
                 if gd is not None:
                     os.kill(gd.proc.pid, 15)
-                print(str(e))
+                log.warning('git-ls-remote failed due to: %s(%s)', type(e), e)
                 if os.name == 'nt':
                     msg = "git-daemon needs to run this test, but windows does not have one. "
                     msg += 'Otherwise, run: git-daemon "%s"' % temp_dir
@@ -239,8 +241,8 @@ def with_rw_and_rw_remote_repo(working_tree_ref):
                 try:
                     return func(self, rw_repo, rw_remote_repo)
                 except:
-                    print("Keeping repos after failure: repo_dir = %s, remote_repo_dir = %s"
-                          % (repo_dir, remote_repo_dir), file=sys.stderr)
+                    log.info("Keeping repos after failure: repo_dir = %s, remote_repo_dir = %s",
+                             repo_dir, remote_repo_dir)
                     repo_dir = remote_repo_dir = None
                     raise
             finally:
diff --git a/git/test/test_git.py b/git/test/test_git.py
index 935673b1..ea62de03 100644
--- a/git/test/test_git.py
+++ b/git/test/test_git.py
@@ -21,7 +21,8 @@ from git import (
     Git,
     GitCommandError,
     GitCommandNotFound,
-    Repo
+    Repo,
+    cmd
 )
 from gitdb.test.lib import with_rw_directory
 
@@ -240,7 +241,7 @@ class TestGit(TestBase):
                                 stderr=subprocess.PIPE,
                                 shell=False,
                                 universal_newlines=True,
-                                creationflags=Git.CREATE_NO_WINDOW if sys.platform == 'win32' else 0,
+                                creationflags=cmd.PROC_CREATIONFLAGS,
                                 )
 
         handle_process_output(proc, counter_stdout, counter_stderr, lambda proc: proc.wait())