From aafb300a8101627540560e5cd73753de42304ca2 Mon Sep 17 00:00:00 2001 From: sroet Date: Fri, 10 Sep 2021 13:36:15 +0200 Subject: [PATCH 01/17] change default fetch timeout to 60 s --- git/cmd.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git/cmd.py b/git/cmd.py index 642ef9ed6..13c5e7a55 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -79,7 +79,7 @@ def handle_process_output(process: 'Git.AutoInterrupt' | Popen, finalizer: Union[None, Callable[[Union[subprocess.Popen, 'Git.AutoInterrupt']], None]] = None, decode_streams: bool = True, - timeout: float = 10.0) -> None: + timeout: float = 60.0) -> None: """Registers for notifications to learn that process output is ready to read, and dispatches lines to the respective line handlers. This function returns once the finalizer returns From 1d265152263b548c296ec5e3f244de8a89377602 Mon Sep 17 00:00:00 2001 From: sroet Date: Fri, 10 Sep 2021 13:45:24 +0200 Subject: [PATCH 02/17] allow for timeout propagation --- git/remote.py | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/git/remote.py b/git/remote.py index 55772f4a0..e3a819cfc 100644 --- a/git/remote.py +++ b/git/remote.py @@ -707,7 +707,8 @@ def update(self, **kwargs: Any) -> 'Remote': return self def _get_fetch_info_from_stderr(self, proc: 'Git.AutoInterrupt', - progress: Union[Callable[..., Any], RemoteProgress, None] + progress: Union[Callable[..., Any], RemoteProgress, None], + timeout: float = 60.0 ) -> IterableList['FetchInfo']: progress = to_progress_instance(progress) @@ -724,7 +725,8 @@ def _get_fetch_info_from_stderr(self, proc: 'Git.AutoInterrupt', cmds = set(FetchInfo._flag_map.keys()) progress_handler = progress.new_message_handler() - handle_process_output(proc, None, progress_handler, finalizer=None, decode_streams=False) + handle_process_output(proc, None, progress_handler, finalizer=None, decode_streams=False, + timeout=timeout) stderr_text = progress.error_lines and '\n'.join(progress.error_lines) or '' proc.wait(stderr=stderr_text) @@ -769,7 +771,8 @@ def _get_fetch_info_from_stderr(self, proc: 'Git.AutoInterrupt', return output def _get_push_info(self, proc: 'Git.AutoInterrupt', - progress: Union[Callable[..., Any], RemoteProgress, None]) -> IterableList[PushInfo]: + progress: Union[Callable[..., Any], RemoteProgress, None], + timeout: float = 60.0) -> IterableList[PushInfo]: progress = to_progress_instance(progress) # read progress information from stderr @@ -786,7 +789,8 @@ def stdout_handler(line: str) -> None: # If an error happens, additional info is given which we parse below. pass - handle_process_output(proc, stdout_handler, progress_handler, finalizer=None, decode_streams=False) + handle_process_output(proc, stdout_handler, progress_handler, finalizer=None, decode_streams=False, + timeout=timeout) stderr_text = progress.error_lines and '\n'.join(progress.error_lines) or '' try: proc.wait(stderr=stderr_text) @@ -813,7 +817,8 @@ def _assert_refspec(self) -> None: def fetch(self, refspec: Union[str, List[str], None] = None, progress: Union[RemoteProgress, None, 'UpdateProgress'] = None, - verbose: bool = True, **kwargs: Any) -> IterableList[FetchInfo]: + verbose: bool = True, timeout: float = 60.0, + **kwargs: Any) -> IterableList[FetchInfo]: """Fetch the latest changes for this remote :param refspec: @@ -853,13 +858,14 @@ def fetch(self, refspec: Union[str, List[str], None] = None, proc = self.repo.git.fetch(self, *args, as_process=True, with_stdout=False, universal_newlines=True, v=verbose, **kwargs) - res = self._get_fetch_info_from_stderr(proc, progress) + res = self._get_fetch_info_from_stderr(proc, progress, timeout=timeout) if hasattr(self.repo.odb, 'update_cache'): self.repo.odb.update_cache() return res def pull(self, refspec: Union[str, List[str], None] = None, progress: Union[RemoteProgress, 'UpdateProgress', None] = None, + timeout: float = 60.0, **kwargs: Any) -> IterableList[FetchInfo]: """Pull changes from the given branch, being the same as a fetch followed by a merge of branch with your local branch. @@ -874,14 +880,14 @@ def pull(self, refspec: Union[str, List[str], None] = None, kwargs = add_progress(kwargs, self.repo.git, progress) proc = self.repo.git.pull(self, refspec, with_stdout=False, as_process=True, universal_newlines=True, v=True, **kwargs) - res = self._get_fetch_info_from_stderr(proc, progress) + res = self._get_fetch_info_from_stderr(proc, progress, timeout=timeout) if hasattr(self.repo.odb, 'update_cache'): self.repo.odb.update_cache() return res def push(self, refspec: Union[str, List[str], None] = None, progress: Union[RemoteProgress, 'UpdateProgress', Callable[..., RemoteProgress], None] = None, - **kwargs: Any) -> IterableList[PushInfo]: + timeout: float = 60.0, **kwargs: Any) -> IterableList[PushInfo]: """Push changes from source branch in refspec to target branch in refspec. :param refspec: see 'fetch' method @@ -909,7 +915,7 @@ def push(self, refspec: Union[str, List[str], None] = None, kwargs = add_progress(kwargs, self.repo.git, progress) proc = self.repo.git.push(self, refspec, porcelain=True, as_process=True, universal_newlines=True, **kwargs) - return self._get_push_info(proc, progress) + return self._get_push_info(proc, progress, timeout=timeout) @ property def config_reader(self) -> SectionConstraint[GitConfigParser]: From febd4fe5854131b928c7e40066dd99b19dd8d76c Mon Sep 17 00:00:00 2001 From: sroet Date: Fri, 10 Sep 2021 13:50:57 +0200 Subject: [PATCH 03/17] add test timeout with the old 10 s timeout --- test/test_remote.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test_remote.py b/test/test_remote.py index c29fac65c..13da128f7 100644 --- a/test/test_remote.py +++ b/test/test_remote.py @@ -401,7 +401,7 @@ def _assert_push_and_pull(self, remote, rw_repo, remote_repo): res = remote.push(all=True) self._do_test_push_result(res, remote) - remote.pull('master') + remote.pull('master', timeout=10.0) # cleanup - delete created tags and branches as we are in an innerloop on # the same repository @@ -467,7 +467,7 @@ def test_base(self, rw_repo, remote_repo): # Only for remotes - local cases are the same or less complicated # as additional progress information will never be emitted if remote.name == "daemon_origin": - self._do_test_fetch(remote, rw_repo, remote_repo) + self._do_test_fetch(remote, rw_repo, remote_repo, timeout=10.0) ran_fetch_test = True # END fetch test From 4113d01725f9c2413282d630c343a7a14e2ce493 Mon Sep 17 00:00:00 2001 From: sroet Date: Fri, 10 Sep 2021 13:57:34 +0200 Subject: [PATCH 04/17] also test a call to 'push' with 10s timeout --- test/test_remote.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_remote.py b/test/test_remote.py index 13da128f7..243eec290 100644 --- a/test/test_remote.py +++ b/test/test_remote.py @@ -406,7 +406,7 @@ def _assert_push_and_pull(self, remote, rw_repo, remote_repo): # cleanup - delete created tags and branches as we are in an innerloop on # the same repository TagReference.delete(rw_repo, new_tag, other_tag) - remote.push(":%s" % other_tag.path) + remote.push(":%s" % other_tag.path, timeout=10.0) @skipIf(HIDE_WINDOWS_FREEZE_ERRORS, "FIXME: Freezes!") @with_rw_and_rw_remote_repo('0.1.6') From c55a8e3e227fc04d10a28f8e8f61809148b19223 Mon Sep 17 00:00:00 2001 From: sroet Date: Fri, 10 Sep 2021 14:05:24 +0200 Subject: [PATCH 05/17] propagate kwargs in do_test_fetch --- test/test_remote.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test_remote.py b/test/test_remote.py index 243eec290..e5fe8dd00 100644 --- a/test/test_remote.py +++ b/test/test_remote.py @@ -164,7 +164,7 @@ def _commit_random_file(self, repo): index.commit("Committing %s" % new_file) return new_file - def _do_test_fetch(self, remote, rw_repo, remote_repo): + def _do_test_fetch(self, remote, rw_repo, remote_repo, **kwargs): # specialized fetch testing to de-clutter the main test self._do_test_fetch_info(rw_repo) @@ -183,7 +183,7 @@ def get_info(res, remote, name): # put remote head to master as it is guaranteed to exist remote_repo.head.reference = remote_repo.heads.master - res = fetch_and_test(remote) + res = fetch_and_test(remote, **kwargs) # all up to date for info in res: self.assertTrue(info.flags & info.HEAD_UPTODATE) From 7df33f3e653ad7b07ca78196066d31d9d03b3972 Mon Sep 17 00:00:00 2001 From: sroet Date: Fri, 10 Sep 2021 17:06:53 +0200 Subject: [PATCH 06/17] reset default timeout to None --- git/cmd.py | 2 +- git/remote.py | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index 13c5e7a55..0deb4ffcc 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -79,7 +79,7 @@ def handle_process_output(process: 'Git.AutoInterrupt' | Popen, finalizer: Union[None, Callable[[Union[subprocess.Popen, 'Git.AutoInterrupt']], None]] = None, decode_streams: bool = True, - timeout: float = 60.0) -> None: + timeout: Union[None, float] = None) -> None: """Registers for notifications to learn that process output is ready to read, and dispatches lines to the respective line handlers. This function returns once the finalizer returns diff --git a/git/remote.py b/git/remote.py index e3a819cfc..ce5d82b5b 100644 --- a/git/remote.py +++ b/git/remote.py @@ -708,7 +708,7 @@ def update(self, **kwargs: Any) -> 'Remote': def _get_fetch_info_from_stderr(self, proc: 'Git.AutoInterrupt', progress: Union[Callable[..., Any], RemoteProgress, None], - timeout: float = 60.0 + timeout: Union[None, float] = None, ) -> IterableList['FetchInfo']: progress = to_progress_instance(progress) @@ -772,7 +772,7 @@ def _get_fetch_info_from_stderr(self, proc: 'Git.AutoInterrupt', def _get_push_info(self, proc: 'Git.AutoInterrupt', progress: Union[Callable[..., Any], RemoteProgress, None], - timeout: float = 60.0) -> IterableList[PushInfo]: + timeout: Union[None, float] = None) -> IterableList[PushInfo]: progress = to_progress_instance(progress) # read progress information from stderr @@ -817,7 +817,7 @@ def _assert_refspec(self) -> None: def fetch(self, refspec: Union[str, List[str], None] = None, progress: Union[RemoteProgress, None, 'UpdateProgress'] = None, - verbose: bool = True, timeout: float = 60.0, + verbose: bool = True, timeout: Union[None, float] = None, **kwargs: Any) -> IterableList[FetchInfo]: """Fetch the latest changes for this remote @@ -865,7 +865,7 @@ def fetch(self, refspec: Union[str, List[str], None] = None, def pull(self, refspec: Union[str, List[str], None] = None, progress: Union[RemoteProgress, 'UpdateProgress', None] = None, - timeout: float = 60.0, + timeout: Union[None, float] = None, **kwargs: Any) -> IterableList[FetchInfo]: """Pull changes from the given branch, being the same as a fetch followed by a merge of branch with your local branch. @@ -887,7 +887,7 @@ def pull(self, refspec: Union[str, List[str], None] = None, def push(self, refspec: Union[str, List[str], None] = None, progress: Union[RemoteProgress, 'UpdateProgress', Callable[..., RemoteProgress], None] = None, - timeout: float = 60.0, **kwargs: Any) -> IterableList[PushInfo]: + timeout: Union[None, float] = None, **kwargs: Any) -> IterableList[PushInfo]: """Push changes from source branch in refspec to target branch in refspec. :param refspec: see 'fetch' method From b555764cfa2afe6eef99b755f1487accbc1c5768 Mon Sep 17 00:00:00 2001 From: sroet Date: Fri, 10 Sep 2021 17:28:00 +0200 Subject: [PATCH 07/17] update docstring --- git/cmd.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git/cmd.py b/git/cmd.py index 0deb4ffcc..f1b3194a3 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -94,7 +94,7 @@ def handle_process_output(process: 'Git.AutoInterrupt' | Popen, their contents to handlers. Set it to False if `universal_newline == True` (then streams are in text-mode) or if decoding must happen later (i.e. for Diffs). - :param timeout: float, timeout to pass to t.join() in case it hangs. Default = 10.0 seconds + :param timeout: float, or None timeout to pass to t.join() in case it hangs. Default = None. """ # Use 2 "pump" threads and wait for both to finish. def pump_stream(cmdline: List[str], name: str, stream: Union[BinaryIO, TextIO], is_decode: bool, From 6b358f9b339bf2d4c9f3d8c00d8e47a43dd0dc82 Mon Sep 17 00:00:00 2001 From: sroet Date: Mon, 13 Sep 2021 17:05:45 +0200 Subject: [PATCH 08/17] reuse kill_after_timeout kwarg --- git/cmd.py | 66 +++++++++++++++++++++++++++++++++++---------------- git/remote.py | 36 +++++++++++++++++++--------- 2 files changed, 71 insertions(+), 31 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index f1b3194a3..db06d5f7c 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -79,7 +79,7 @@ def handle_process_output(process: 'Git.AutoInterrupt' | Popen, finalizer: Union[None, Callable[[Union[subprocess.Popen, 'Git.AutoInterrupt']], None]] = None, decode_streams: bool = True, - timeout: Union[None, float] = None) -> None: + kill_after_timeout: Union[None, float] = None) -> None: """Registers for notifications to learn that process output is ready to read, and dispatches lines to the respective line handlers. This function returns once the finalizer returns @@ -94,7 +94,10 @@ def handle_process_output(process: 'Git.AutoInterrupt' | Popen, their contents to handlers. Set it to False if `universal_newline == True` (then streams are in text-mode) or if decoding must happen later (i.e. for Diffs). - :param timeout: float, or None timeout to pass to t.join() in case it hangs. Default = None. + :param kill_after_timeout: + float or None, Default = None + To specify a timeout in seconds for the git command, after which the process + should be killed. """ # Use 2 "pump" threads and wait for both to finish. def pump_stream(cmdline: List[str], name: str, stream: Union[BinaryIO, TextIO], is_decode: bool, @@ -108,9 +111,12 @@ def pump_stream(cmdline: List[str], name: str, stream: Union[BinaryIO, TextIO], handler(line_str) else: handler(line) + except Exception as ex: log.error(f"Pumping {name!r} of cmd({remove_password_if_present(cmdline)}) failed due to: {ex!r}") - raise CommandError([f'<{name}-pump>'] + remove_password_if_present(cmdline), ex) from ex + if "I/O operation on closed file" not in str(ex): + # Only reraise if the error was not due to the stream closing + raise CommandError([f'<{name}-pump>'] + remove_password_if_present(cmdline), ex) from ex finally: stream.close() @@ -146,9 +152,16 @@ def pump_stream(cmdline: List[str], name: str, stream: Union[BinaryIO, TextIO], ## FIXME: Why Join?? Will block if `stdin` needs feeding... # for t in threads: - t.join(timeout=timeout) + t.join(timeout=kill_after_timeout) if t.is_alive(): - raise RuntimeError(f"Thread join() timed out in cmd.handle_process_output(). Timeout={timeout} seconds") + if hasattr(process, 'proc'): # Assume it is a Git.AutoInterrupt: + process._terminate() + else: # Don't want to deal with the other case + raise RuntimeError(f"Thread join() timed out in cmd.handle_process_output()." + " kill_after_timeout={kill_after_timeout} seconds") + if stderr_handler: + stderr_handler("error: process killed because it timed out." + f" kill_after_timeout={kill_after_timeout} seconds") if finalizer: return finalizer(process) @@ -386,13 +399,15 @@ class AutoInterrupt(object): The wait method was overridden to perform automatic status code checking and possibly raise.""" - __slots__ = ("proc", "args") + __slots__ = ("proc", "args", "status") def __init__(self, proc: Union[None, subprocess.Popen], args: Any) -> None: self.proc = proc self.args = args + self.status = None - def __del__(self) -> None: + def _terminate(self) -> None: + """Terminate the underlying process""" if self.proc is None: return @@ -408,6 +423,7 @@ def __del__(self) -> None: # did the process finish already so we have a return code ? try: if proc.poll() is not None: + self.status = proc.poll() return None except OSError as ex: log.info("Ignored error after process had died: %r", ex) @@ -419,7 +435,7 @@ def __del__(self) -> None: # try to kill it try: proc.terminate() - proc.wait() # ensure process goes away + self.status = proc.wait() # ensure process goes away except OSError as ex: log.info("Ignored error after process had died: %r", ex) except AttributeError: @@ -431,6 +447,11 @@ def __del__(self) -> None: call(("TASKKILL /F /T /PID %s 2>nul 1>nul" % str(proc.pid)), shell=True) # END exception handling + + + def __del__(self) -> None: + self._terminate() + def __getattr__(self, attr: str) -> Any: return getattr(self.proc, attr) @@ -447,21 +468,26 @@ def wait(self, stderr: Union[None, str, bytes] = b'') -> int: if self.proc is not None: status = self.proc.wait() + p_stderr = self.proc.stderr + else: #Assume the underlying proc was killed earlier or never existed + status = self.status + p_stderr = None - def read_all_from_possibly_closed_stream(stream: Union[IO[bytes], None]) -> bytes: - if stream: - try: - return stderr_b + force_bytes(stream.read()) - except ValueError: - return stderr_b or b'' - else: + def read_all_from_possibly_closed_stream(stream: Union[IO[bytes], None]) -> bytes: + if stream: + try: + return stderr_b + force_bytes(stream.read()) + except ValueError: return stderr_b or b'' + else: + return stderr_b or b'' - if status != 0: - errstr = read_all_from_possibly_closed_stream(self.proc.stderr) - log.debug('AutoInterrupt wait stderr: %r' % (errstr,)) - raise GitCommandError(remove_password_if_present(self.args), status, errstr) # END status handling + + if status != 0: + errstr = read_all_from_possibly_closed_stream(p_stderr) + log.debug('AutoInterrupt wait stderr: %r' % (errstr,)) + raise GitCommandError(remove_password_if_present(self.args), status, errstr) return status # END auto interrupt @@ -694,7 +720,7 @@ def execute(self, as_process: bool = False, output_stream: Union[None, BinaryIO] = None, stdout_as_string: bool = True, - kill_after_timeout: Union[None, int] = None, + kill_after_timeout: Union[None, float] = None, with_stdout: bool = True, universal_newlines: bool = False, shell: Union[None, bool] = None, diff --git a/git/remote.py b/git/remote.py index ce5d82b5b..bfa4db592 100644 --- a/git/remote.py +++ b/git/remote.py @@ -708,7 +708,7 @@ def update(self, **kwargs: Any) -> 'Remote': def _get_fetch_info_from_stderr(self, proc: 'Git.AutoInterrupt', progress: Union[Callable[..., Any], RemoteProgress, None], - timeout: Union[None, float] = None, + kill_after_timeout: Union[None, float] = None, ) -> IterableList['FetchInfo']: progress = to_progress_instance(progress) @@ -726,7 +726,7 @@ def _get_fetch_info_from_stderr(self, proc: 'Git.AutoInterrupt', progress_handler = progress.new_message_handler() handle_process_output(proc, None, progress_handler, finalizer=None, decode_streams=False, - timeout=timeout) + kill_after_timeout=kill_after_timeout) stderr_text = progress.error_lines and '\n'.join(progress.error_lines) or '' proc.wait(stderr=stderr_text) @@ -772,7 +772,7 @@ def _get_fetch_info_from_stderr(self, proc: 'Git.AutoInterrupt', def _get_push_info(self, proc: 'Git.AutoInterrupt', progress: Union[Callable[..., Any], RemoteProgress, None], - timeout: Union[None, float] = None) -> IterableList[PushInfo]: + kill_after_timeout: Union[None, float] = None) -> IterableList[PushInfo]: progress = to_progress_instance(progress) # read progress information from stderr @@ -790,7 +790,7 @@ def stdout_handler(line: str) -> None: pass handle_process_output(proc, stdout_handler, progress_handler, finalizer=None, decode_streams=False, - timeout=timeout) + kill_after_timeout=kill_after_timeout) stderr_text = progress.error_lines and '\n'.join(progress.error_lines) or '' try: proc.wait(stderr=stderr_text) @@ -817,7 +817,8 @@ def _assert_refspec(self) -> None: def fetch(self, refspec: Union[str, List[str], None] = None, progress: Union[RemoteProgress, None, 'UpdateProgress'] = None, - verbose: bool = True, timeout: Union[None, float] = None, + verbose: bool = True, + kill_after_timeout: Union[None, float] = None, **kwargs: Any) -> IterableList[FetchInfo]: """Fetch the latest changes for this remote @@ -838,6 +839,9 @@ def fetch(self, refspec: Union[str, List[str], None] = None, for 'refspec' will make use of this facility. :param progress: See 'push' method :param verbose: Boolean for verbose output + :param kill_after_timeout: + To specify a timeout in seconds for the git command, after which the process + should be killed. It is set to None by default. :param kwargs: Additional arguments to be passed to git-fetch :return: IterableList(FetchInfo, ...) list of FetchInfo instances providing detailed @@ -858,20 +862,22 @@ def fetch(self, refspec: Union[str, List[str], None] = None, proc = self.repo.git.fetch(self, *args, as_process=True, with_stdout=False, universal_newlines=True, v=verbose, **kwargs) - res = self._get_fetch_info_from_stderr(proc, progress, timeout=timeout) + res = self._get_fetch_info_from_stderr(proc, progress, + kill_after_timeout=kill_after_timeout) if hasattr(self.repo.odb, 'update_cache'): self.repo.odb.update_cache() return res def pull(self, refspec: Union[str, List[str], None] = None, progress: Union[RemoteProgress, 'UpdateProgress', None] = None, - timeout: Union[None, float] = None, + kill_after_timeout: Union[None, float] = None, **kwargs: Any) -> IterableList[FetchInfo]: """Pull changes from the given branch, being the same as a fetch followed by a merge of branch with your local branch. :param refspec: see 'fetch' method :param progress: see 'push' method + :param kill_after_timeout: see 'fetch' method :param kwargs: Additional arguments to be passed to git-pull :return: Please see 'fetch' method """ if refspec is None: @@ -880,14 +886,16 @@ def pull(self, refspec: Union[str, List[str], None] = None, kwargs = add_progress(kwargs, self.repo.git, progress) proc = self.repo.git.pull(self, refspec, with_stdout=False, as_process=True, universal_newlines=True, v=True, **kwargs) - res = self._get_fetch_info_from_stderr(proc, progress, timeout=timeout) + res = self._get_fetch_info_from_stderr(proc, progress, + kill_after_timeout=kill_after_timeout) if hasattr(self.repo.odb, 'update_cache'): self.repo.odb.update_cache() return res def push(self, refspec: Union[str, List[str], None] = None, progress: Union[RemoteProgress, 'UpdateProgress', Callable[..., RemoteProgress], None] = None, - timeout: Union[None, float] = None, **kwargs: Any) -> IterableList[PushInfo]: + kill_after_timeout: Union[None, float] = None, + **kwargs: Any) -> IterableList[PushInfo]: """Push changes from source branch in refspec to target branch in refspec. :param refspec: see 'fetch' method @@ -903,6 +911,9 @@ def push(self, refspec: Union[str, List[str], None] = None, overrides the ``update()`` function. :note: No further progress information is returned after push returns. + :param kill_after_timeout: + To specify a timeout in seconds for the git command, after which the process + should be killed. It is set to None by default. :param kwargs: Additional arguments to be passed to git-push :return: list(PushInfo, ...) list of PushInfo instances, each @@ -914,8 +925,11 @@ def push(self, refspec: Union[str, List[str], None] = None, be 0.""" kwargs = add_progress(kwargs, self.repo.git, progress) proc = self.repo.git.push(self, refspec, porcelain=True, as_process=True, - universal_newlines=True, **kwargs) - return self._get_push_info(proc, progress, timeout=timeout) + universal_newlines=True, + kill_after_timeout=kill_after_timeout, + **kwargs) + return self._get_push_info(proc, progress, + kill_after_timeout=kill_after_timeout) @ property def config_reader(self) -> SectionConstraint[GitConfigParser]: From 3a81850800018c1c0423423261ac79964d2d7192 Mon Sep 17 00:00:00 2001 From: sroet Date: Mon, 13 Sep 2021 17:59:24 +0200 Subject: [PATCH 09/17] update tests and add a comment about different behaviour of 'push' vs 'fetch' --- git/remote.py | 2 ++ test/test_remote.py | 20 +++++++++++++++++--- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/git/remote.py b/git/remote.py index bfa4db592..9917c4310 100644 --- a/git/remote.py +++ b/git/remote.py @@ -795,6 +795,8 @@ def stdout_handler(line: str) -> None: try: proc.wait(stderr=stderr_text) except Exception: + # This is different than fetch (which fails if there is any std_err + # even if there is an output) if not output: raise elif stderr_text: diff --git a/test/test_remote.py b/test/test_remote.py index e5fe8dd00..1cbc2eb21 100644 --- a/test/test_remote.py +++ b/test/test_remote.py @@ -6,6 +6,7 @@ import random import tempfile +import pytest from unittest import skipIf from git import ( @@ -401,12 +402,12 @@ def _assert_push_and_pull(self, remote, rw_repo, remote_repo): res = remote.push(all=True) self._do_test_push_result(res, remote) - remote.pull('master', timeout=10.0) + remote.pull('master', kill_after_timeout=10.0) # cleanup - delete created tags and branches as we are in an innerloop on # the same repository TagReference.delete(rw_repo, new_tag, other_tag) - remote.push(":%s" % other_tag.path, timeout=10.0) + remote.push(":%s" % other_tag.path, kill_after_timeout=10.0) @skipIf(HIDE_WINDOWS_FREEZE_ERRORS, "FIXME: Freezes!") @with_rw_and_rw_remote_repo('0.1.6') @@ -467,7 +468,8 @@ def test_base(self, rw_repo, remote_repo): # Only for remotes - local cases are the same or less complicated # as additional progress information will never be emitted if remote.name == "daemon_origin": - self._do_test_fetch(remote, rw_repo, remote_repo, timeout=10.0) + self._do_test_fetch(remote, rw_repo, remote_repo, + kill_after_timeout=10.0) ran_fetch_test = True # END fetch test @@ -651,3 +653,15 @@ def test_push_error(self, repo): rem = repo.remote('origin') with self.assertRaisesRegex(GitCommandError, "src refspec __BAD_REF__ does not match any"): rem.push('__BAD_REF__') + + +class TestTimeouts(TestBase): + @with_rw_repo('HEAD', bare=False) + def test_timeout_funcs(self, repo): + for function in ["pull", "fetch"]: #"can't get push to reliably timeout + f = getattr(repo.remotes.origin, function) + assert f is not None # Make sure these functions exist + + with self.assertRaisesRegex(GitCommandError, + "kill_after_timeout=0.01 s"): + f(kill_after_timeout=0.01) From d41d5378353d5045064a1143a349b728c6f2dd80 Mon Sep 17 00:00:00 2001 From: sroet Date: Mon, 13 Sep 2021 18:04:27 +0200 Subject: [PATCH 10/17] go for pytest.raises and test that the functions run --- test/test_remote.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/test_remote.py b/test/test_remote.py index 1cbc2eb21..10f0bb4bd 100644 --- a/test/test_remote.py +++ b/test/test_remote.py @@ -661,7 +661,7 @@ def test_timeout_funcs(self, repo): for function in ["pull", "fetch"]: #"can't get push to reliably timeout f = getattr(repo.remotes.origin, function) assert f is not None # Make sure these functions exist - - with self.assertRaisesRegex(GitCommandError, - "kill_after_timeout=0.01 s"): + _ = f() # Make sure the function runs + with pytest.raises(GitCommandError, + match="kill_after_timeout=0.01 s"): f(kill_after_timeout=0.01) From 55313237acd12067592b8082d02582ec09dfa160 Mon Sep 17 00:00:00 2001 From: sroet Date: Tue, 14 Sep 2021 12:27:17 +0200 Subject: [PATCH 11/17] make flake8 and mypy happy --- git/cmd.py | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index db06d5f7c..7523ead57 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -154,14 +154,22 @@ def pump_stream(cmdline: List[str], name: str, stream: Union[BinaryIO, TextIO], for t in threads: t.join(timeout=kill_after_timeout) if t.is_alive(): - if hasattr(process, 'proc'): # Assume it is a Git.AutoInterrupt: + if isinstance(process, Git.AutoInterrupt): process._terminate() else: # Don't want to deal with the other case - raise RuntimeError(f"Thread join() timed out in cmd.handle_process_output()." - " kill_after_timeout={kill_after_timeout} seconds") + raise RuntimeError("Thread join() timed out in cmd.handle_process_output()." + f" kill_after_timeout={kill_after_timeout} seconds") if stderr_handler: - stderr_handler("error: process killed because it timed out." - f" kill_after_timeout={kill_after_timeout} seconds") + error_str: Union[str, bytes] = ( + "error: process killed because it timed out." + f" kill_after_timeout={kill_after_timeout} seconds") + if not decode_streams and isinstance(p_stderr, BinaryIO): + # Assume stderr_handler needs binary input + error_str = cast(str, error_str) + error_str = error_str.encode() + # We ignore typing on the next line because mypy does not like + # the way we infered that stderr takes str of bytes + stderr_handler(error_str) # type: ignore if finalizer: return finalizer(process) @@ -404,7 +412,7 @@ class AutoInterrupt(object): def __init__(self, proc: Union[None, subprocess.Popen], args: Any) -> None: self.proc = proc self.args = args - self.status = None + self.status: Union[int, None] = None def _terminate(self) -> None: """Terminate the underlying process""" @@ -447,8 +455,6 @@ def _terminate(self) -> None: call(("TASKKILL /F /T /PID %s 2>nul 1>nul" % str(proc.pid)), shell=True) # END exception handling - - def __del__(self) -> None: self._terminate() @@ -465,11 +471,11 @@ def wait(self, stderr: Union[None, str, bytes] = b'') -> int: if stderr is None: stderr_b = b'' stderr_b = force_bytes(data=stderr, encoding='utf-8') - + status: Union[int, None] if self.proc is not None: status = self.proc.wait() p_stderr = self.proc.stderr - else: #Assume the underlying proc was killed earlier or never existed + else: # Assume the underlying proc was killed earlier or never existed status = self.status p_stderr = None From da68bb1fef7e2797f8dd4aa41be94c447abddf2e Mon Sep 17 00:00:00 2001 From: sroet Date: Tue, 14 Sep 2021 12:34:23 +0200 Subject: [PATCH 12/17] fix typo's --- git/cmd.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git/cmd.py b/git/cmd.py index 7523ead57..9279bb0c3 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -168,7 +168,7 @@ def pump_stream(cmdline: List[str], name: str, stream: Union[BinaryIO, TextIO], error_str = cast(str, error_str) error_str = error_str.encode() # We ignore typing on the next line because mypy does not like - # the way we infered that stderr takes str of bytes + # the way we inferred that stderr takes str or bytes stderr_handler(error_str) # type: ignore if finalizer: From 96782266d4e4140414a12ffc470e82f14ac3cb02 Mon Sep 17 00:00:00 2001 From: sroet Date: Tue, 14 Sep 2021 13:30:33 +0200 Subject: [PATCH 13/17] make test timeout stricter --- test/test_remote.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test_remote.py b/test/test_remote.py index 10f0bb4bd..8f0206646 100644 --- a/test/test_remote.py +++ b/test/test_remote.py @@ -663,5 +663,5 @@ def test_timeout_funcs(self, repo): assert f is not None # Make sure these functions exist _ = f() # Make sure the function runs with pytest.raises(GitCommandError, - match="kill_after_timeout=0.01 s"): - f(kill_after_timeout=0.01) + match="kill_after_timeout=0 s"): + f(kill_after_timeout=0) From a9e43e521e57125e9f27ec5d6c04723039038636 Mon Sep 17 00:00:00 2001 From: sroet Date: Tue, 14 Sep 2021 13:59:22 +0200 Subject: [PATCH 14/17] fetch is also to quick on CI, only test pull --- test/test_remote.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_remote.py b/test/test_remote.py index 8f0206646..9fe649ad7 100644 --- a/test/test_remote.py +++ b/test/test_remote.py @@ -658,7 +658,7 @@ def test_push_error(self, repo): class TestTimeouts(TestBase): @with_rw_repo('HEAD', bare=False) def test_timeout_funcs(self, repo): - for function in ["pull", "fetch"]: #"can't get push to reliably timeout + for function in ["pull"]: #"can't get fetch and push to reliably timeout f = getattr(repo.remotes.origin, function) assert f is not None # Make sure these functions exist _ = f() # Make sure the function runs From 50818813c17e082c452a3854fa56dd1c5775460f Mon Sep 17 00:00:00 2001 From: sroet Date: Tue, 14 Sep 2021 14:00:30 +0200 Subject: [PATCH 15/17] two spaces before comments --- test/test_remote.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_remote.py b/test/test_remote.py index 9fe649ad7..4b06a88ac 100644 --- a/test/test_remote.py +++ b/test/test_remote.py @@ -658,7 +658,7 @@ def test_push_error(self, repo): class TestTimeouts(TestBase): @with_rw_repo('HEAD', bare=False) def test_timeout_funcs(self, repo): - for function in ["pull"]: #"can't get fetch and push to reliably timeout + for function in ["pull"]: # can't get fetch and push to reliably timeout f = getattr(repo.remotes.origin, function) assert f is not None # Make sure these functions exist _ = f() # Make sure the function runs From 083039a7a84f12798ecd6073dbd8ce5db6a46817 Mon Sep 17 00:00:00 2001 From: sroet Date: Tue, 14 Sep 2021 14:09:29 +0200 Subject: [PATCH 16/17] set timeout to a non-zero value --- test/test_remote.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test_remote.py b/test/test_remote.py index 4b06a88ac..4c1d02c86 100644 --- a/test/test_remote.py +++ b/test/test_remote.py @@ -663,5 +663,5 @@ def test_timeout_funcs(self, repo): assert f is not None # Make sure these functions exist _ = f() # Make sure the function runs with pytest.raises(GitCommandError, - match="kill_after_timeout=0 s"): - f(kill_after_timeout=0) + match="kill_after_timeout=0.001 s"): + f(kill_after_timeout=0.001) From e058c4c2e681b5c5ae3f59ae912c0700f83802f3 Mon Sep 17 00:00:00 2001 From: sroet Date: Wed, 15 Sep 2021 11:55:17 +0200 Subject: [PATCH 17/17] Add a way to force status codes inside AutoInterrupt._terminate, and let tests use it --- git/cmd.py | 19 ++++++++++++------- test/test_remote.py | 14 ++++++++++---- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index 9279bb0c3..8fb10742f 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -409,6 +409,10 @@ class AutoInterrupt(object): __slots__ = ("proc", "args", "status") + # If this is non-zero it will override any status code during + # _terminate, used to prevent race conditions in testing + _status_code_if_terminate: int = 0 + def __init__(self, proc: Union[None, subprocess.Popen], args: Any) -> None: self.proc = proc self.args = args @@ -427,11 +431,10 @@ def _terminate(self) -> None: proc.stdout.close() if proc.stderr: proc.stderr.close() - # did the process finish already so we have a return code ? try: if proc.poll() is not None: - self.status = proc.poll() + self.status = self._status_code_if_terminate or proc.poll() return None except OSError as ex: log.info("Ignored error after process had died: %r", ex) @@ -443,7 +446,9 @@ def _terminate(self) -> None: # try to kill it try: proc.terminate() - self.status = proc.wait() # ensure process goes away + status = proc.wait() # ensure process goes away + + self.status = self._status_code_if_terminate or status except OSError as ex: log.info("Ignored error after process had died: %r", ex) except AttributeError: @@ -849,7 +854,7 @@ def execute(self, if is_win: cmd_not_found_exception = OSError - if kill_after_timeout: + if kill_after_timeout is not None: raise GitCommandError(redacted_command, '"kill_after_timeout" feature is not supported on Windows.') else: cmd_not_found_exception = FileNotFoundError # NOQA # exists, flake8 unknown @UndefinedVariable @@ -916,7 +921,7 @@ def _kill_process(pid: int) -> None: return # end - if kill_after_timeout: + if kill_after_timeout is not None: kill_check = threading.Event() watchdog = threading.Timer(kill_after_timeout, _kill_process, args=(proc.pid,)) @@ -927,10 +932,10 @@ def _kill_process(pid: int) -> None: newline = "\n" if universal_newlines else b"\n" try: if output_stream is None: - if kill_after_timeout: + if kill_after_timeout is not None: watchdog.start() stdout_value, stderr_value = proc.communicate() - if kill_after_timeout: + if kill_after_timeout is not None: watchdog.cancel() if kill_check.is_set(): stderr_value = ('Timeout: the command "%s" did not complete in %d ' diff --git a/test/test_remote.py b/test/test_remote.py index 4c1d02c86..088fdad55 100644 --- a/test/test_remote.py +++ b/test/test_remote.py @@ -658,10 +658,16 @@ def test_push_error(self, repo): class TestTimeouts(TestBase): @with_rw_repo('HEAD', bare=False) def test_timeout_funcs(self, repo): - for function in ["pull"]: # can't get fetch and push to reliably timeout + # Force error code to prevent a race condition if the python thread is + # slow + default = Git.AutoInterrupt._status_code_if_terminate + Git.AutoInterrupt._status_code_if_terminate = -15 + for function in ["pull", "fetch"]: # can't get push to timeout f = getattr(repo.remotes.origin, function) assert f is not None # Make sure these functions exist - _ = f() # Make sure the function runs + _ = f() # Make sure the function runs with pytest.raises(GitCommandError, - match="kill_after_timeout=0.001 s"): - f(kill_after_timeout=0.001) + match="kill_after_timeout=0 s"): + f(kill_after_timeout=0) + + Git.AutoInterrupt._status_code_if_terminate = default