Repository: gitpython-developers/GitPython

Issue:
default thread timeout too short for CI
Since this morning [our CI](https://github.com/openpathsampling/openpathsampling/runs/3563720388?) started failing with:

```
Run python devtools/autorelease_check.py
Traceback (most recent call last):
  File "devtools/autorelease_check.py", line 24, in <module>
    repo_path='.'
  File "/usr/share/miniconda/envs/test/lib/python3.7/site-packages/autorelease/check_runners.py", line 52, in __init__
    repo_path=repo_path
  File "/usr/share/miniconda/envs/test/lib/python3.7/site-packages/autorelease/git_repo_checks.py", line 28, in __init__
    self.repo.remotes.origin.fetch()
  File "/usr/share/miniconda/envs/test/lib/python3.7/site-packages/git/remote.py", line 856, in fetch
    res = self._get_fetch_info_from_stderr(proc, progress)
  File "/usr/share/miniconda/envs/test/lib/python3.7/site-packages/git/remote.py", line 727, in _get_fetch_info_from_stderr
    handle_process_output(proc, None, progress_handler, finalizer=None, decode_streams=False)
  File "/usr/share/miniconda/envs/test/lib/python3.7/site-packages/git/cmd.py", line 151, in handle_process_output
    raise RuntimeError(f"Thread join() timed out in cmd.handle_process_output(). Timeout={timeout} seconds")
RuntimeError: Thread join() timed out in cmd.handle_process_output(). Timeout=10.0 seconds
Fatal Python error: could not acquire lock for <_io.BufferedReader name=4> at interpreter shutdown, possibly due to daemon threads

Thread 0x00007fbb2f2d7700 (most recent call first):
  File "/usr/share/miniconda/envs/test/lib/python3.7/site-packages/git/cmd.py", line 103 in pump_stream
  File "/usr/share/miniconda/envs/test/lib/python3.7/threading.py", line 870 in run
  File "/usr/share/miniconda/envs/test/lib/python3.7/threading.py", line 926 in _bootstrap_inner
  File "/usr/share/miniconda/envs/test/lib/python3.7/threading.py", line 890 in _bootstrap

Current thread 0x00007fbb3332d180 (most recent call first):
  File "/usr/share/miniconda/envs/test/lib/python3.7/site-packages/git/cmd.py", line 406 in __del__
/home/runner/work/_temp/ff824c06-ff56-4ee4-9e2a-e65258688cf4.sh: line 1:  1922 Aborted                 (core dumped) python devtools/autorelease_check.py
Error: Process completed with exit code 134.
```

This seems a side effect of the timeout introduced by #1318 

Could this time-out default to `60 s` instead (or be taken and propagated by the `fetch` function)?


I could make a PR if needed, with either of the two solutions

  


Discussion:
sroet: alright, the test suite runs locally and gives the following coverage:
```
---------- coverage: platform linux, python 3.7.11-final-0 -----------
Name                                Stmts   Miss  Cover
-------------------------------------------------------
git/__init__.py                        42      8    81%
git/cmd.py                            525     98    81%
git/compat.py                          46     15    67%
git/config.py                         439     48    89%
git/db.py                              27      1    96%
git/diff.py                           268     10    96%
git/exc.py                             63      3    95%
git/index/__init__.py                   2      0   100%
git/index/base.py                     498     32    94%
git/index/fun.py                      179      7    96%
git/index/typ.py                       68      3    96%
git/index/util.py                      50      5    90%
git/objects/__init__.py                13      0   100%
git/objects/base.py                    91     10    89%
git/objects/blob.py                    17      0   100%
git/objects/commit.py                 276     28    90%
git/objects/fun.py                    101      5    95%
git/objects/submodule/__init__.py       0      0   100%
git/objects/submodule/base.py         583     46    92%
git/objects/submodule/root.py         137     12    91%
git/objects/submodule/util.py          51      9    82%
git/objects/tag.py                     47     12    74%
git/objects/tree.py                   181     73    60%
git/objects/util.py                   232     32    86%
git/refs/__init__.py                    6      0   100%
git/refs/head.py                       98      6    94%
git/refs/log.py                       150     10    93%
git/refs/reference.py                  55      3    95%
git/refs/remote.py                     32      2    94%
git/refs/symbolic.py                  338     18    95%
git/refs/tag.py                        47      6    87%
git/remote.py                         427     55    87%
git/repo/__init__.py                    1      0   100%
git/repo/base.py                      552     36    93%
git/repo/fun.py                       201     20    90%
git/types.py                           44     10    77%
git/util.py                           539    121    78%
-------------------------------------------------------
TOTAL                                6426    744    88%


Results (64.31s):
     378 passed
      42 skipped
```
Byron: Thanks a lot for the initiative. With the offending release yanked I believe we win some time to wait for @Yobmod to chime in on this.

My preference, if I understand what's happening correctly, is to set the default timeout to `None` and not alter the previous behaviour, as the code as is can't tell if it's just a long-running process or a stalled one. Thus I would prefer offering this up to callers only.
Yobmod: Hi, I think setting the timeout default to None works better, rather than a larger number.
Thread.join(None) would then wait as long as needed, but if people want a timeout, it's there to use.

Or it could be removed altogether.
It got added when the tests started hanging in a 3.10 beta  I couldn't find the problem, but it doesn't happen with release candidates any more.

dwhswenson: FWIW, the one concern I have about surfacing the argument here is that I think there might be confusion between `cmd.execute_kwargs['kill_after_timeout']` and this `timeout`. As a user, without a careful read of the docs I would expect the behavior of `remote.fetch(timeout=60)` to be that of `'kill_after_timeout'`.
sroet: @Yobmod, @dwhswenson  ; I have reverted the default to None, please let me know what you think.

@Byron,  I will not be able to push to this branch for the next ~60 hour, so feel free to push to it if you want this to be finished up before monday 
Byron: @sroet Thanks for your help with this and the incredibly quick turnaround time, and for everyone to share their thoughts to make this better.

> 

> FWIW, the one concern I have about surfacing the argument here is that I think there might be confusion between `cmd.execute_kwargs['kill_after_timeout']` and this `timeout`. As a user, without a careful read of the docs I would expect the behavior of `remote.fetch(timeout=60)` to be that of `'kill_after_timeout'`.

To me it seems that `kill_after_timeout` could have been used to handle this all along, as killing the process the threads are reading from should break their pipes and thus stop the read-loops, allowing them to stop naturally. Since we have tests for this maybe you could test this hypothesis. Indeed it would be great to avoid two timeout parameters who have the same intention, but only one of them seems to be solving the issue at its root.

The actual process termination [happens here](https://github.com/gitpython-developers/GitPython/blob/main/git/cmd.py#L859:L880) and it looks like it will succeed on non-windows but might still fail to kill stalled processes on windows. To my mind, that's acceptable and I'd rather wait for users to report issues due to this than to keep the `timeout` parameter around just for the case the git process isn't responding to signals.
sroet: This took a bit more work than expected, but should now work with the keyword `kill_after_timeout` for the user facing functions

> The actual process termination happens here and it looks like it will succeed on non-windows but might still fail to kill stalled processes on windows. To my mind, that's acceptable and I'd rather wait for users to report issues due to this than to keep the timeout parameter around just for the case the git process isn't responding to signals.


That did not really work out, as most user facing functions (`fetch`, `pull`, and `push`) set `as_process=True` so that code path is never hit, instead [this if statement is hit](https://github.com/gitpython-developers/GitPython/blob/main/git/cmd.py#L856:L857) which returns an `AutoInterrupt` object. Looking at [that class](https://github.com/gitpython-developers/GitPython/blob/main/git/cmd.py#L395:L432) it actually knows how to kill underlying processes (in the `__del__` method) so I decided to factor that code out to a private function `_terminate` (and fixing `wait` if `self.proc=None`). 

>To me it seems that kill_after_timeout could have been used to handle this all along, as killing the process the threads are reading from should break their pipes and thus stop the read-loops, allowing them to stop naturally.

The reading threads had no way of handling the closing of the streams, so added a check to the exception. 

It should now raises like this:
```python
>>> a = git.Repo('.')
>>> a.remotes.origin.fetch(kill_after_timeout=0.01)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/sander/github_files/GitPython/git/remote.py", line 868, in fetch
    kill_after_timeout=kill_after_timeout)
  File "/home/sander/github_files/GitPython/git/remote.py", line 732, in _get_fetch_info_from_stderr
    proc.wait(stderr=stderr_text)
  File "/home/sander/github_files/GitPython/git/cmd.py", line 490, in wait
    raise GitCommandError(remove_password_if_present(self.args), status, errstr)
git.exc.GitCommandError: Cmd('git') failed due to: exit code(-15)
  cmdline: git fetch -v origin
  stderr: 'error: process killed because it timed out. kill_after_timeout=0.01 seconds'
```

When writing the tests, I saw that the error handling code of `push` does something different than for `fetch` and `pull`

- `fetch`/`pull` always error out  if there is any error lines in `std_err`(it does not care if there is output as that is piped to `None`)
- `push` only errors out if there are error lines in `std_err` and *no* output to `std_out`

@Byron Is this mismatch intentional/wanted?

I will update the initial post to indicate the current status




Byron: Thanks a million for all the effort you are putting in!

> @Byron Is this mismatch intentional/wanted?

It's based on the observation on how these processes usually work and communicate errors. However, if it feels out-of-place or wrong there is no reason not to improve it while you are here. It wouldn't have been the first time that there are long-standing issues in the code-base that somehow never surfaced. While I encourage you for these fixes I also warn that the test-suite might not catch all the subtle behaviour either, so it's probably better to leave as much unchanged as possible while fixing the issue at hand.

Note that CI is failing because of the linter and white-space related errors. It's strange that it asks me to approve running CI on this PR over and over again and I hope it will just run when the fix is pushed.
dwhswenson: > It's strange that it asks me to approve running CI on this PR over and over again and I hope it will just run when the fix is pushed.

See https://github.blog/2021-04-22-github-actions-update-helping-maintainers-combat-bad-actors/, in particular:

> Once a collaborator with write access has reviewed the changes, they can approve the workflows to run for the current commit only. If the pull request is updated to a new commit, a new approval will be required.

IMO, that last is a bad choice by GitHub. I understand the desire to avoid abuse by crypto-miners, but there should be an option for the approval to be for the duration of the PR.
Byron: Thanks for the heads-up, I wasn't aware of this change.

> […] but there should be an option for the approval to be for the duration of the PR.

Couldn't agree more. Here it seems they are over-steering towards abuse-avoidance and trade convenience of using their platform. Instead, if legitimate PRs turn out to become abusive later, it's something I'd happily report - after it happened.
Maybe additional options will be implemented some day, I do have hope.
sroet: @bryon, sorry, I should have checked mypy and flake8 localy. they now return:
```
(gitpy) ***@***:~/github_files/GitPython$ flake8
0
(gitpy) ***@***:~/github_files/GitPython$ mypy -p git
Success: no issues found in 37 source files
```
sroet: Looking at the test failure for 9678226, it seems like the CI environment is too quick `fetching` after doing a `pull` to reliably triggering the case, only doing a pull should still hit most codepaths...
sroet: Hmm, it started failing locally with timeout=0 , so reset it to a small value. Hopefully this should be small enough to at least let CI fail out, otherwise I would like a reviewer to see if the test also fails on their local system...
sroet: A bit of info on why the error is not raised but still shows up in the log as:
```
WARNING  git.remote:remote.py:734 Error lines received while fetching: error: process killed because it timed out. kill_after_timeout=0 seconds
```
This is because there is a race condition: in the time it takes between the main thread triggering the timeout (which gives the log) and telling the process to terminate (which should trigger the error by setting the status to 0), the process can complete correctly. In this case, I decided of letting the code recover instead of erroring out.
Byron: It's interesting as CI still fails even though the timeout is small. Can there be a 'too-small' due to same race elsewhere?

Maybe the CI is onto something here and indicates some fixable issue, and for now I'd be inclined to believe that.
sroet: @Byron the issue lies in the fact that the CI python tread is too slow compared to the git process, I can force CI behavior locally by altering the lines:
```python
    for t in threads:                                                           
        t.join(timeout=kill_after_timeout)                                      
        if t.is_alive():                                                        
            if isinstance(process, Git.AutoInterrupt):                                                                          
                process._terminate()                                            
```
to
```python
    for t in threads:                                                           
        t.join(timeout=kill_after_timeout)                                      
        if t.is_alive():                                                        
            if isinstance(process, Git.AutoInterrupt):                          
                #TODO debug code                                                
                import time                                                     
                time.sleep(1)                                                   
                process._terminate()                                            
```

What happens is that inside the `AutoInterupt._terminate() function the following lines exist:
```python
            # did the process finish already so we have a return code ?         
            try:                                                                
                if proc.poll() is not None:                                     
                    self.status = proc.poll()                                   
                    return None   
```
which allows the process status to be `0` even if the timeout is triggered initially. 

and even later it still does:
```
            # try to kill it                                                    
            try:                                                                
                proc.terminate()                                                
                self.status = proc.wait()    # ensure process goes away    
```
Which also allows the process to terminate gracefully if the kill signal is not fast enough.

Now, I can force set `self.status to `-15` inside the `_terminate` function which make sure we always trigger the error whenever `_terminate()` has been called, even if the process completed between python triggering a timeout and getting around to actually kill the process.

 @Byron is that (not allowing the process to recover) the preferred solution here?



Byron: > @Byron is that (not allowing the process to recover) the preferred solution here?

A robust implementation is preferred any time, and it seems like a system that can recover to states that are changing is the better option.

Ultimately CI should work and ideally work for the next few years without spurious failures due to races.
sroet: > Ultimately CI should work and ideally work for the next few years without spurious failures due to races.

Added a class variable to `AutoInterrupt` that sets the statuscode inside `_terminate` if it non-zero, and updated the tests to use it. It makes the test pass locally, with the `sleep(1)`. So I am pretty confident it catches the race condition in tests and still allows for the robust implementation, please have another look

Patch:
diff --git a/git/cmd.py b/git/cmd.py
index 642ef9ed..8fb10742 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:
+                          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, timeout to pass to t.join() in case it hangs. Default = 10.0 seconds
+    :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 handle_process_output(process: 'Git.AutoInterrupt' | Popen,
                         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,24 @@ def handle_process_output(process: 'Git.AutoInterrupt' | Popen,
     ## 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 isinstance(process, Git.AutoInterrupt):
+                process._terminate()
+            else:  # Don't want to deal with the other case
+                raise RuntimeError("Thread join() timed out in cmd.handle_process_output()."
+                                   f" kill_after_timeout={kill_after_timeout} seconds")
+            if stderr_handler:
+                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 inferred that stderr takes str or bytes
+                stderr_handler(error_str)  # type: ignore
 
     if finalizer:
         return finalizer(process)
@@ -386,13 +407,19 @@ class Git(LazyMixin):
         The wait method was overridden to perform automatic status code checking
         and possibly raise."""
 
-        __slots__ = ("proc", "args")
+        __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
+            self.status: Union[int, None] = None
 
-        def __del__(self) -> None:
+        def _terminate(self) -> None:
+            """Terminate the underlying process"""
             if self.proc is None:
                 return
 
@@ -404,10 +431,10 @@ class Git(LazyMixin):
                 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 = self._status_code_if_terminate or proc.poll()
                     return None
             except OSError as ex:
                 log.info("Ignored error after process had died: %r", ex)
@@ -419,7 +446,9 @@ class Git(LazyMixin):
             # try to kill it
             try:
                 proc.terminate()
-                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:
@@ -431,6 +460,9 @@ class Git(LazyMixin):
                     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)
 
@@ -444,24 +476,29 @@ class Git(LazyMixin):
             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
+                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 +731,7 @@ class Git(LazyMixin):
                 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,
@@ -817,7 +854,7 @@ class Git(LazyMixin):
 
         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
@@ -884,7 +921,7 @@ class Git(LazyMixin):
             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,))
 
@@ -895,10 +932,10 @@ class Git(LazyMixin):
         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/git/remote.py b/git/remote.py
index 55772f4a..9917c431 100644
--- a/git/remote.py
+++ b/git/remote.py
@@ -707,7 +707,8 @@ class Remote(LazyMixin, IterableObj):
         return self
 
     def _get_fetch_info_from_stderr(self, proc: 'Git.AutoInterrupt',
-                                    progress: Union[Callable[..., Any], RemoteProgress, None]
+                                    progress: Union[Callable[..., Any], RemoteProgress, None],
+                                    kill_after_timeout: Union[None, float] = None,
                                     ) -> IterableList['FetchInfo']:
 
         progress = to_progress_instance(progress)
@@ -724,7 +725,8 @@ class Remote(LazyMixin, IterableObj):
         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,
+                              kill_after_timeout=kill_after_timeout)
 
         stderr_text = progress.error_lines and '\n'.join(progress.error_lines) or ''
         proc.wait(stderr=stderr_text)
@@ -769,7 +771,8 @@ class Remote(LazyMixin, IterableObj):
         return output
 
     def _get_push_info(self, proc: 'Git.AutoInterrupt',
-                       progress: Union[Callable[..., Any], RemoteProgress, None]) -> IterableList[PushInfo]:
+                       progress: Union[Callable[..., Any], RemoteProgress, None],
+                       kill_after_timeout: Union[None, float] = None) -> IterableList[PushInfo]:
         progress = to_progress_instance(progress)
 
         # read progress information from stderr
@@ -786,11 +789,14 @@ class Remote(LazyMixin, IterableObj):
                 # 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,
+                              kill_after_timeout=kill_after_timeout)
         stderr_text = progress.error_lines and '\n'.join(progress.error_lines) or ''
         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:
@@ -813,7 +819,9 @@ class Remote(LazyMixin, IterableObj):
 
     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,
+              kill_after_timeout: Union[None, float] = None,
+              **kwargs: Any) -> IterableList[FetchInfo]:
         """Fetch the latest changes for this remote
 
         :param refspec:
@@ -833,6 +841,9 @@ class Remote(LazyMixin, IterableObj):
             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
@@ -853,19 +864,22 @@ class Remote(LazyMixin, IterableObj):
 
         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,
+                                               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,
+             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:
@@ -874,13 +888,15 @@ class Remote(LazyMixin, IterableObj):
         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,
+                                               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,
+             kill_after_timeout: Union[None, float] = None,
              **kwargs: Any) -> IterableList[PushInfo]:
         """Push changes from source branch in refspec to target branch in refspec.
 
@@ -897,6 +913,9 @@ class Remote(LazyMixin, IterableObj):
               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
@@ -908,8 +927,11 @@ class Remote(LazyMixin, IterableObj):
             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)
+                                  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]:


Test patch:
diff --git a/test/test_remote.py b/test/test_remote.py
index c29fac65..088fdad5 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 (
@@ -164,7 +165,7 @@ class TestRemote(TestBase):
         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 +184,7 @@ class TestRemote(TestBase):
         # 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)
@@ -401,12 +402,12 @@ class TestRemote(TestBase):
         res = remote.push(all=True)
         self._do_test_push_result(res, remote)
 
-        remote.pull('master')
+        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)
+        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 @@ class TestRemote(TestBase):
             # 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,
+                                    kill_after_timeout=10.0)
                 ran_fetch_test = True
             # END fetch test
 
@@ -651,3 +653,21 @@ class TestRemote(TestBase):
         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):
+        # 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
+            with pytest.raises(GitCommandError,
+                               match="kill_after_timeout=0 s"):
+                f(kill_after_timeout=0)
+
+        Git.AutoInterrupt._status_code_if_terminate = default