fixup! Use subprocess's timeout feature instead of reinventing the wheel

Author: ngie-eign

git/cmd.py

@@ -109,7 +109,7 @@ def handle_process_output(
     stderr_handler: Union[None, Callable[[AnyStr], None], Callable[[List[AnyStr]], None]],
     finalizer: Union[None, Callable[[Union[Popen, "Git.AutoInterrupt"]], None]] = None,
     decode_streams: bool = True,
-    kill_after_timeout: Optional[float, int] = None,
+    kill_after_timeout: Optional[Union[float, int]] = None,
 ) -> None:
     R"""Register for notifications to learn that process output is ready to read, and
     dispatch lines to the respective line handlers.
@@ -335,7 +335,7 @@ def __init__(
         self,
         proc: subprocess.Popen | None,
         args: Any,
-        timeout: Optional[float, int] = None,
+        timeout: Optional[Union[float, int]] = None,
     ) -> None:
         self.proc = proc
         self.args = args
@@ -370,10 +370,15 @@ def _terminate(self) -> None:
         # Try to kill it.
         try:
             proc.terminate()
-            status = proc.wait(timeout=self.timeout)  # Ensure the process goes away.
+            status = proc.wait(timeout=self.timeout)  # Gracefully wait for the process to terminate.
 
             self.status = self._status_code_if_terminate or status
-        except (OSError, AttributeError, subprocess.TimeoutExpired) as ex:
+        except subprocess.TimeoutExpired:
+            _logger.warning("Process did not complete successfully in %s seconds; will forcefully kill", exc_info=True)
+            proc.kill()
+            status = proc.wait()  # Ensure the process goes away by blocking with `timeout=None`.
+            self.status = self._status_code_if_terminate or status
+        except (OSError, AttributeError) as ex:
             # On interpreter shutdown (notably on Windows), parts of the stdlib used by
             # subprocess can already be torn down (e.g. `subprocess._winapi` becomes None),
             # which can cause AttributeError during terminate(). In that case, we prefer
@@ -389,7 +394,7 @@ def __getattr__(self, attr: str) -> Any:
         return getattr(self.proc, attr)
 
     # TODO: Bad choice to mimic `proc.wait()` but with different args.
-    def wait(self, stderr: Union[None, str, bytes] = b"") -> int:
+    def wait(self, stderr: Optional[Union[str, bytes]] = b"", timeout: Optional[Union[int, float]] = None) -> int:
         """Wait for the process and return its status code.
 
         :param stderr:
@@ -406,7 +411,8 @@ def wait(self, stderr: Union[None, str, bytes] = b"") -> int:
         stderr_b = force_bytes(data=stderr, encoding="utf-8")
         status: Union[int, None]
         if self.proc is not None:
-            status = self.proc.wait(timeout=self.timeout)
+            timeout = self.timeout if timeout is None else timeout
+            status = self.proc.wait(timeout=timeout)
             p_stderr = self.proc.stderr
         else:  # Assume the underlying proc was killed earlier or never existed.
             status = self.status
@@ -1112,7 +1118,7 @@ def execute(
         as_process: bool = False,
         output_stream: Union[None, BinaryIO] = None,
         stdout_as_string: bool = True,
-        kill_after_timeout: Optional[float, int] = None,
+        kill_after_timeout: Optional[Union[float, int]] = None,
         with_stdout: bool = True,
         universal_newlines: bool = False,
         shell: Union[None, bool] = None,
@@ -1325,8 +1331,9 @@ def execute(
                 "error: process killed because it timed out. kill_after_timeout=%s seconds",
                 kill_after_timeout,
             )
-            proc.terminate()
-            stdout_value, stderr_value = proc.communicate()
+            with contextlib.suppress(subprocess.TimeoutExpired):
+                proc.kill()
+                stdout_value, stderr_value = proc.communicate(timeout=0.1)
             if with_exceptions:
                 raise GitCommandError(redacted_command, proc.returncode, stderr_value, stdout_value) from err
         finally:

git/remote.py

@@ -875,7 +875,7 @@ def _get_fetch_info_from_stderr(
         self,
         proc: "Git.AutoInterrupt",
         progress: Union[Callable[..., Any], RemoteProgress, None],
-        kill_after_timeout: Optional[float, int] = None,
+        kill_after_timeout: Optional[Union[float, int]] = None,
     ) -> IterableList["FetchInfo"]:
         progress = to_progress_instance(progress)
 
@@ -946,7 +946,7 @@ def _get_push_info(
         self,
         proc: "Git.AutoInterrupt",
         progress: Union[Callable[..., Any], RemoteProgress, None],
-        kill_after_timeout: Optional[float, int] = None,
+        kill_after_timeout: Optional[Union[float, int]] = None,
     ) -> PushInfoList:
         progress = to_progress_instance(progress)
 
@@ -1004,7 +1004,7 @@ def fetch(
         refspec: Union[str, List[str], None] = None,
         progress: Union[RemoteProgress, None, "UpdateProgress"] = None,
         verbose: bool = True,
-        kill_after_timeout: Optional[float, int] = None,
+        kill_after_timeout: Optional[Union[float, int]] = None,
         allow_unsafe_protocols: bool = False,
         allow_unsafe_options: bool = False,
         **kwargs: Any,
@@ -1084,7 +1084,7 @@ def pull(
         self,
         refspec: Union[str, List[str], None] = None,
         progress: Union[RemoteProgress, "UpdateProgress", None] = None,
-        kill_after_timeout: Optional[float, int] = None,
+        kill_after_timeout: Optional[Union[float, int]] = None,
         allow_unsafe_protocols: bool = False,
         allow_unsafe_options: bool = False,
         **kwargs: Any,
@@ -1138,7 +1138,7 @@ def push(
         self,
         refspec: Union[str, List[str], None] = None,
         progress: Union[RemoteProgress, "UpdateProgress", Callable[..., RemoteProgress], None] = None,
-        kill_after_timeout: Optional[float, int] = None,
+        kill_after_timeout: Optional[Union[float, int]] = None,
         allow_unsafe_protocols: bool = False,
         allow_unsafe_options: bool = False,
         **kwargs: Any,