From cd4caf15a2e977fc0f010c1532090d942421979c Mon Sep 17 00:00:00 2001 From: Oswin Nathanial Date: Mon, 28 Sep 2015 11:23:52 +0900 Subject: [PATCH 1/6] Include 'timeout' parameter in Git execute This feature enables to set a timeout while executing a git command. After this timeout is over, the process will be killed. If not explicitly specified, the default functionality will not be affected. Change-Id: I2dd5f0de7cb1f5f1b4253dd7ce92d23551d5f9a7 --- git/cmd.py | 40 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/git/cmd.py b/git/cmd.py index 3cdc68ab7..f093ca73d 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -14,6 +14,7 @@ import mmap from contextlib import contextmanager +from signal import SIGKILL from subprocess import ( call, Popen, @@ -41,7 +42,7 @@ execute_kwargs = ('istream', 'with_keep_cwd', 'with_extended_output', 'with_exceptions', 'as_process', 'stdout_as_string', - 'output_stream', 'with_stdout') + 'output_stream', 'with_stdout', 'timeout') log = logging.getLogger('git.cmd') log.addHandler(logging.NullHandler()) @@ -475,6 +476,7 @@ def execute(self, command, as_process=False, output_stream=None, stdout_as_string=True, + timeout=None, with_stdout=True, **subprocess_kwargs ): @@ -531,6 +533,12 @@ def execute(self, command, :param with_stdout: If True, default True, we open stdout on the created process + :param timeout: + To specify a timeout in seconds for the git command, after which the process + should be killed. This will have no effect if as_process is set to True. It is + set to None by default and will let the process run until the timeout is + explicitly specified. + :return: * str(output) if extended_output = False (Default) * tuple(int(status), str(stdout), str(stderr)) if extended_output = True @@ -592,13 +600,43 @@ def execute(self, command, if as_process: return self.AutoInterrupt(proc, command) + kill_check = threading.Event() + + def _kill_process(pid): + """ Callback method to kill a process. """ + p = Popen(['ps', '--ppid', str(pid)], stdout=PIPE) + child_pids = [] + for line in p.stdout: + if len(line.split()) > 0: + local_pid = (line.split())[0] + if local_pid.isdigit(): + child_pids.append(int(local_pid)) + try: + os.kill(pid, SIGKILL) + for child_pid in child_pids: + os.kill(child_pid, SIGKILL) + kill_check.set() # tell the main routine that the process was killed + except OSError: + # It is possible that the process gets completed in the duration after timeout + # happens and before we try to kill the process. + pass + return + # end + + watchdog = threading.Timer(timeout, _kill_process, args=(proc.pid, )) + # Wait for the process to return status = 0 stdout_value = b'' stderr_value = b'' try: if output_stream is None: + watchdog.start() stdout_value, stderr_value = proc.communicate() + watchdog.cancel() + if kill_check.isSet(): + stderr_value = 'Timeout: the command "%s" did not complete in %d ' \ + 'secs.' % (" ".join(command), timeout) # strip trailing "\n" if stdout_value.endswith(b"\n"): stdout_value = stdout_value[:-1] From 9e1c90eb69e2dfd5fdf8418caa695112bd285f21 Mon Sep 17 00:00:00 2001 From: Oswin Nathanial Date: Fri, 9 Oct 2015 15:18:27 +0900 Subject: [PATCH 2/6] Raise exception when timeout is used in execute command on Windows Change-Id: I2e081c606b75b7f8d3d1ee82d93c3d9f3bdcfcbe --- git/cmd.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/git/cmd.py b/git/cmd.py index f093ca73d..fd3e815b9 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -576,6 +576,8 @@ def execute(self, command, if sys.platform == 'win32': cmd_not_found_exception = WindowsError + if timeout: + raise GitCommandError('"timeout" feature is not supported on Windows.') else: if sys.version_info[0] > 2: cmd_not_found_exception = FileNotFoundError # NOQA # this is defined, but flake8 doesn't know From 7cc0e6caa3117f694d367d3f3b80db1e365aac94 Mon Sep 17 00:00:00 2001 From: Oswin Nathanial Date: Fri, 9 Oct 2015 15:22:28 +0900 Subject: [PATCH 3/6] Only create watchdog and event if timeout is specified in execute command If the timeout is not specified, we don't need the overhead of creating a watchdog and event. Change-Id: I53ff891af24d4c27fb16bf4bb35910dd1d19d238 --- git/cmd.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index fd3e815b9..392f3a0be 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -602,8 +602,6 @@ def execute(self, command, if as_process: return self.AutoInterrupt(proc, command) - kill_check = threading.Event() - def _kill_process(pid): """ Callback method to kill a process. """ p = Popen(['ps', '--ppid', str(pid)], stdout=PIPE) @@ -625,7 +623,9 @@ def _kill_process(pid): return # end - watchdog = threading.Timer(timeout, _kill_process, args=(proc.pid, )) + if timeout: + kill_check = threading.Event() + watchdog = threading.Timer(timeout, _kill_process, args=(proc.pid, )) # Wait for the process to return status = 0 @@ -633,12 +633,14 @@ def _kill_process(pid): stderr_value = b'' try: if output_stream is None: - watchdog.start() + if timeout: + watchdog.start() stdout_value, stderr_value = proc.communicate() - watchdog.cancel() - if kill_check.isSet(): - stderr_value = 'Timeout: the command "%s" did not complete in %d ' \ - 'secs.' % (" ".join(command), timeout) + if timeout: + watchdog.cancel() + if kill_check.isSet(): + stderr_value = 'Timeout: the command "%s" did not complete in %d ' \ + 'secs.' % (" ".join(command), timeout) # strip trailing "\n" if stdout_value.endswith(b"\n"): stdout_value = stdout_value[:-1] From 4faf5cd43dcd0b3eea0a3e71077c21f4d029eb99 Mon Sep 17 00:00:00 2001 From: Oswin Nathanial Date: Tue, 13 Oct 2015 10:48:00 +0900 Subject: [PATCH 4/6] Rename execute param 'timeout' to 'kill_after_timeout' Change-Id: I8ab3d5affb3f040dd9630687fb20aedbd7510070 --- git/cmd.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index 392f3a0be..4b267206f 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -42,7 +42,7 @@ execute_kwargs = ('istream', 'with_keep_cwd', 'with_extended_output', 'with_exceptions', 'as_process', 'stdout_as_string', - 'output_stream', 'with_stdout', 'timeout') + 'output_stream', 'with_stdout', 'kill_after_timeout') log = logging.getLogger('git.cmd') log.addHandler(logging.NullHandler()) @@ -476,7 +476,7 @@ def execute(self, command, as_process=False, output_stream=None, stdout_as_string=True, - timeout=None, + kill_after_timeout=None, with_stdout=True, **subprocess_kwargs ): @@ -533,7 +533,7 @@ def execute(self, command, :param with_stdout: If True, default True, we open stdout on the created process - :param timeout: + :param kill_after_timeout: To specify a timeout in seconds for the git command, after which the process should be killed. This will have no effect if as_process is set to True. It is set to None by default and will let the process run until the timeout is @@ -576,8 +576,8 @@ def execute(self, command, if sys.platform == 'win32': cmd_not_found_exception = WindowsError - if timeout: - raise GitCommandError('"timeout" feature is not supported on Windows.') + if kill_after_timeout: + raise GitCommandError('"kill_after_timeout" feature is not supported on Windows.') else: if sys.version_info[0] > 2: cmd_not_found_exception = FileNotFoundError # NOQA # this is defined, but flake8 doesn't know @@ -623,9 +623,9 @@ def _kill_process(pid): return # end - if timeout: + if kill_after_timeout: kill_check = threading.Event() - watchdog = threading.Timer(timeout, _kill_process, args=(proc.pid, )) + watchdog = threading.Timer(kill_after_timeout, _kill_process, args=(proc.pid, )) # Wait for the process to return status = 0 @@ -633,14 +633,14 @@ def _kill_process(pid): stderr_value = b'' try: if output_stream is None: - if timeout: + if kill_after_timeout: watchdog.start() stdout_value, stderr_value = proc.communicate() - if timeout: + if kill_after_timeout: watchdog.cancel() if kill_check.isSet(): stderr_value = 'Timeout: the command "%s" did not complete in %d ' \ - 'secs.' % (" ".join(command), timeout) + 'secs.' % (" ".join(command), kill_after_timeout) # strip trailing "\n" if stdout_value.endswith(b"\n"): stdout_value = stdout_value[:-1] From d06e76bb243dda3843cfaefe7adc362aab2b7215 Mon Sep 17 00:00:00 2001 From: Oswin Nathanial Date: Tue, 13 Oct 2015 10:56:22 +0900 Subject: [PATCH 5/6] Update docstring for 'kill_after_timeout' parameter Specify that this feature is not supported on Windows and mention about the negative side-effects of SIGKILL on a repository. Change-Id: Ibba2c3f51f84084b4637ae9aaafa87dd84000ef4 --- git/cmd.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/git/cmd.py b/git/cmd.py index 4b267206f..a7435497f 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -537,7 +537,11 @@ def execute(self, command, To specify a timeout in seconds for the git command, after which the process should be killed. This will have no effect if as_process is set to True. It is set to None by default and will let the process run until the timeout is - explicitly specified. + explicitly specified. This feature is not supported on Windows. It's also worth + noting that kill_after_timeout uses SIGKILL, which can have negative side + effects on a repository. For example, stale locks in case of git gc could + render the repository incapable of accepting changes until the lock is manually + removed. :return: * str(output) if extended_output = False (Default) From dbbcaf7a355e925911fa77e204dd2c38ee633c0f Mon Sep 17 00:00:00 2001 From: Oswin Nathanial Date: Tue, 13 Oct 2015 11:51:05 +0900 Subject: [PATCH 6/6] Run os.kill for all child pids even after some of them are down Right now, we come out of the iteration in case of failure while trying to kill a child pid. This may result in some of the child pids staying alive. Change-Id: I18d58fcefec2bbdae4ae9bf73594939ade241b52 --- git/cmd.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/git/cmd.py b/git/cmd.py index a7435497f..af019f138 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -618,7 +618,10 @@ def _kill_process(pid): try: os.kill(pid, SIGKILL) for child_pid in child_pids: - os.kill(child_pid, SIGKILL) + try: + os.kill(child_pid, SIGKILL) + except OSError: + pass kill_check.set() # tell the main routine that the process was killed except OSError: # It is possible that the process gets completed in the duration after timeout