Skip to content

Use subprocess timeout #2127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ngie-eign wants to merge 3 commits into
base: main
Choose a base branch
from
+72 −71
Open

Use subprocess timeout #2127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d966a0d
git.cmd.Git.execute(..): fix `with_stdout=False`
ngie-eign Apr 17, 2026
6fc4742
test_avoids_changing...: don't leave test artifacts behind
ngie-eign Apr 17, 2026
14190cb
Use subprocess's timeout feature instead of reinventing the wheel
ngie-eign Apr 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,5 +56,6 @@ Contributors are:
-Ethan Lin <et.repositories _at_ gmail.com>
-Jonas Scharpf <jonas.scharpf _at_ checkmk.com>
-Gordon Marx
-Enji Cooper

Portions derived from other open source works and are clearly marked.
95 changes: 26 additions & 69 deletions git/cmd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
import logging
import os
import re
import signal
import subprocess
from subprocess import DEVNULL, PIPE, Popen
import sys
Expand Down Expand Up @@ -110,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: Union[None, float] = None,
kill_after_timeout: float | int | None = None,
) -> None:
R"""Register for notifications to learn that process output is ready to read, and
dispatch lines to the respective line handlers.
Expand Down Expand Up @@ -139,7 +138,7 @@ def handle_process_output(
- decoding must happen later, such as for :class:`~git.diff.Diff`\s.

:param kill_after_timeout:
:class:`float` or ``None``, Default = ``None``
:class:`int`, `float`, or ``None`` (block indefinitely), Default = ``None``.
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reST markup in the kill_after_timeout parameter doc is inconsistent (:class:int, float, or ...). Consider using literal markup consistently (e.g., int, float, None) to avoid broken generated docs.

Suggested change
:class:`int`, `float`, or ``None`` (block indefinitely), Default = ``None``.
``int``, ``float``, or ``None`` (block indefinitely), Default = ``None``.

Copilot uses AI. Check for mistakes.

To specify a timeout in seconds for the git command, after which the process
should be killed.
Expand Down Expand Up @@ -326,16 +325,22 @@ class _AutoInterrupt:
raise.
"""

__slots__ = ("proc", "args", "status")
__slots__ = ("proc", "args", "status", "timeout")

# 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:
def __init__(
self,
proc: subprocess.Popen | None,
args: Any,
timeout: float | int | None = None,
) -> None:
self.proc = proc
self.args = args
self.status: Union[int, None] = None
self.timeout = timeout

def _terminate(self) -> None:
"""Terminate the underlying process."""
Expand Down Expand Up @@ -365,7 +370,7 @@ def _terminate(self) -> None:
# Try to kill it.
try:
proc.terminate()
status = proc.wait() # Ensure the process goes away.
status = proc.wait(timeout=self.timeout) # Ensure the process goes away.
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_AutoInterrupt._terminate() now calls proc.wait(timeout=self.timeout). If a timeout is ever set, wait() can raise subprocess.TimeoutExpired, which is currently unhandled and could leave the process running (and self.proc already cleared). Consider catching TimeoutExpired and escalating to proc.kill() + a final wait() (possibly without a timeout) so termination is reliable.

Suggested change
status = proc.wait(timeout=self.timeout) # Ensure the process goes away.
try:
status = proc.wait(timeout=self.timeout) # Ensure the process goes away.
except subprocess.TimeoutExpired:
proc.kill()
status = proc.wait()

Copilot uses AI. Check for mistakes.

self.status = self._status_code_if_terminate or status
except (OSError, AttributeError) as ex:
Expand Down Expand Up @@ -400,7 +405,7 @@ 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()
status = self.proc.wait(timeout=self.timeout)
p_stderr = self.proc.stderr
else: # Assume the underlying proc was killed earlier or never existed.
status = self.status
Expand Down Expand Up @@ -1303,86 +1308,38 @@ def execute(
if as_process:
return self.AutoInterrupt(proc, command)

if sys.platform != "win32" and kill_after_timeout is not None:
# Help mypy figure out this is not None even when used inside communicate().
timeout = kill_after_timeout

def kill_process(pid: int) -> None:
"""Callback to kill a process.

This callback implementation would be ineffective and unsafe on Windows.
"""
p = Popen(["ps", "--ppid", str(pid)], stdout=PIPE)
child_pids = []
if p.stdout is not None:
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, signal.SIGKILL)
for child_pid in child_pids:
try:
os.kill(child_pid, signal.SIGKILL)
except OSError:
pass
# Tell the main routine that the process was killed.
kill_check.set()
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

def communicate() -> Tuple[AnyStr, AnyStr]:
watchdog.start()
out, err = proc.communicate()
watchdog.cancel()
if kill_check.is_set():
err = 'Timeout: the command "%s" did not complete in %d secs.' % (
" ".join(redacted_command),
timeout,
)
if not universal_newlines:
err = err.encode(defenc)
return out, err

# END helpers

kill_check = threading.Event()
watchdog = threading.Timer(timeout, kill_process, args=(proc.pid,))
else:
communicate = proc.communicate

# Wait for the process to return.
status = 0
stdout_value: Union[str, bytes] = b""
stderr_value: Union[str, bytes] = b""
newline = "\n" if universal_newlines else b"\n"
try:
if output_stream is None:
stdout_value, stderr_value = communicate()
stdout_value, stderr_value = proc.communicate(timeout=kill_after_timeout)
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is intended to enable kill_after_timeout via the built-in timeout= support, including on Windows (per PR description), but Git.execute still rejects kill_after_timeout on Windows and the docstring still claims it is unsupported. Either the Windows guard/docs should be updated/removed, or the PR description should be adjusted to match the actual behavior.

Copilot uses AI. Check for mistakes.
# Strip trailing "\n".
Comment on lines +1318 to 1319
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are existing timeout tests for remote operations, but this change introduces new timeout behavior in Git.execute itself (e.g., communicate(timeout=...) / wait(timeout=...), handling of TimeoutExpired, behavior with output_stream). Consider adding/adjusting unit tests that exercise Git.execute(kill_after_timeout=...) directly, including the output_stream branch, to prevent regressions.

Copilot uses AI. Check for mistakes.
Comment on lines +1318 to 1319
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

proc.communicate(timeout=kill_after_timeout) can raise subprocess.TimeoutExpired, but that exception is not handled here. That means the process may be left running and callers will see an unexpected exception rather than the documented/previous behavior of killing the process and surfacing a GitCommandError with useful stderr. Consider catching TimeoutExpired, terminating/killing the process, collecting any remaining output, and then raising GitCommandError (or otherwise translating the timeout) consistently.

Copilot uses AI. Check for mistakes.
if stdout_value.endswith(newline) and strip_newline_in_stdout: # type: ignore[arg-type]
if stdout_value is not None and stdout_value.endswith(newline) and strip_newline_in_stdout: # type: ignore[arg-type]
stdout_value = stdout_value[:-1]
if stderr_value.endswith(newline): # type: ignore[arg-type]
if stderr_value is not None and stderr_value.endswith(newline): # type: ignore[arg-type]
stderr_value = stderr_value[:-1]

status = proc.returncode
else:
max_chunk_size = max_chunk_size if max_chunk_size and max_chunk_size > 0 else io.DEFAULT_BUFFER_SIZE
stream_copy(proc.stdout, output_stream, max_chunk_size)
stdout_value = proc.stdout.read()
stderr_value = proc.stderr.read()
if proc.stdout is not None:
stream_copy(proc.stdout, output_stream, max_chunk_size)
stdout_value = proc.stdout.read()
Comment on lines 1327 to +1330
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When output_stream is provided, the timeout is only applied to proc.wait(...) after stream_copy(...) finishes. If the child process hangs (or blocks writing) stream_copy can block indefinitely and kill_after_timeout will never be enforced. To preserve timeout semantics, consider using communicate(timeout=...) in this branch (writing captured stdout to output_stream), or implementing a non-blocking/selector-based copy loop that enforces the deadline while reading.

Copilot uses AI. Check for mistakes.
if proc.stderr is not None:
stderr_value = proc.stderr.read()
Comment on lines +1330 to +1332
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the output_stream branch, if with_stdout=False then proc.stdout is None and stdout_value remains the initial b"". In the non-output_stream branch, communicate() would return None for stdout in that case. Consider explicitly setting stdout_value = None when proc.stdout is None to keep return values consistent across branches.

Suggested change
stdout_value = proc.stdout.read()
if proc.stderr is not None:
stderr_value = proc.stderr.read()
stdout_value = proc.stdout.read()
else:
stdout_value = None
if proc.stderr is not None:
stderr_value = proc.stderr.read()
else:
stderr_value = None

Copilot uses AI. Check for mistakes.
# Strip trailing "\n".
if stderr_value.endswith(newline): # type: ignore[arg-type]
if stderr_value is not None and stderr_value.endswith(newline): # type: ignore[arg-type]
stderr_value = stderr_value[:-1]
status = proc.wait()
status = proc.wait(timeout=kill_after_timeout)
# END stdout handling
finally:
proc.stdout.close()
proc.stderr.close()
if proc.stdout is not None:
proc.stdout.close()
if proc.stderr is not None:
proc.stderr.close()

if self.GIT_PYTHON_TRACE == "full":
cmdstr = " ".join(redacted_command)
Expand Down
20 changes: 20 additions & 0 deletions test/test_git.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import contextlib
import gc
import inspect
import io
import logging
import os
import os.path as osp
Expand Down Expand Up @@ -201,6 +202,25 @@ def test_it_logs_istream_summary_for_stdin(self, case):
def test_it_executes_git_and_returns_result(self):
self.assertRegex(self.git.execute(["git", "version"]), r"^git version [\d\.]{2}.*$")

def test_it_output_stream_with_stdout_is_false(self):
temp_stream = io.BytesIO()
self.git.execute(
["git", "version"],
output_stream=temp_stream,
with_stdout=False,
)
self.assertEqual(temp_stream.tell(), 0)

def test_it_executes_git_without_stdout_redirect(self):
returncode, stdout, stderr = self.git.execute(
["git", "version"],
with_extended_output=True,
with_stdout=False,
)
self.assertEqual(returncode, 0)
self.assertIsNone(stdout)
self.assertIsNotNone(stderr)

@ddt.data(
# chdir_to_repo, shell, command, use_shell_impostor
(False, False, ["git", "version"], False),
Expand Down
27 changes: 25 additions & 2 deletions test/test_util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ def test_deletes_dir_with_readonly_files(self, tmp_path):
sys.platform == "cygwin",
reason="Cygwin can't set the permissions that make the test meaningful.",
)
def test_avoids_changing_permissions_outside_tree(self, tmp_path):
def test_avoids_changing_permissions_outside_tree(self, tmp_path, request):
# Automatically works on Windows, but on Unix requires either special handling
# or refraining from attempting to fix PermissionError by making chmod calls.

Expand All @@ -125,9 +125,32 @@ def test_avoids_changing_permissions_outside_tree(self, tmp_path):

dir2 = tmp_path / "dir2"
dir2.mkdir()
(dir2 / "symlink").symlink_to(dir1 / "file")
symlink = dir2 / "symlink"
symlink.symlink_to(dir1 / "file")
dir2.chmod(stat.S_IRUSR | stat.S_IXUSR)

def preen_dir2():
"""Don't leave unwritable directories behind.

pytest has difficulties cleaning up after the fact on some platforms,
e.g., macOS, and whines incessantly until the issue is resolved--regardless
of the pytest session.
"""
rwx = stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR
if not dir2.exists():
return
if symlink.exists():
try:
# Try lchmod first, if the platform supports it.
symlink.lchmod(rwx)
except NotImplementedError:
# The platform (probably win32) doesn't support lchmod; fall back to chmod.
Comment on lines +144 to +147
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pathlib.Path does not provide a lchmod() method on supported Python versions; this will raise AttributeError in the finalizer on platforms where this path is hit. If you need to chmod a symlink itself, use os.lchmod(...) where available, or Path.chmod(..., follow_symlinks=False) on Python versions that support it, with an AttributeError/NotImplementedError fallback.

Suggested change
# Try lchmod first, if the platform supports it.
symlink.lchmod(rwx)
except NotImplementedError:
# The platform (probably win32) doesn't support lchmod; fall back to chmod.
# Try to chmod the symlink itself, if the platform supports it.
if hasattr(os, "lchmod"):
os.lchmod(symlink, rwx)
else:
symlink.chmod(rwx, follow_symlinks=False)
except (AttributeError, NotImplementedError, TypeError):
# Fall back to chmod if symlink-specific chmod is unavailable.

Copilot uses AI. Check for mistakes.
symlink.chmod(rwx)
dir2.chmod(rwx)
rmtree(dir2)

request.addfinalizer(preen_dir2)

try:
rmtree(dir2)
except PermissionError:
Expand Down
Loading