Skip to content

Commit

Permalink
Refactor kill_after_timeout logic so mypy can check it
Browse files Browse the repository at this point in the history
This changes how Git.execute defines the kill_process callback and
how it performs checks, fixing two mypy errors on Windows about how
the signal module doesn't have SIGKILL. In doing so, it also
eliminates the need for the assertion added for safety and clarity
in 2f017ac (gitpython-developers#1761), since now kill_process is only defined if it is
to be used (which is also guarded by a platform check, needed by
mypy).

As in dc95a76 before this, part of the change here is to replace
some os.named-based checks with sys.platform-based checks, which is
safe because, when one is specifically checking only for the
distinction between native Windows and all other systems, one can
use either approach. (See dc95a76 for more details on that.)
  • Loading branch information
EliahKagan committed Mar 7, 2024
1 parent dc95a76 commit 4191f7d
Showing 1 changed file with 35 additions and 32 deletions.
67 changes: 35 additions & 32 deletions git/cmd.py
Original file line number Diff line number Diff line change
Expand Up @@ -1134,7 +1134,7 @@ def execute(
if inline_env is not None:
env.update(inline_env)

if os.name == "nt":
if sys.platform == "win32":
cmd_not_found_exception = OSError
if kill_after_timeout is not None:
raise GitCommandError(
Expand Down Expand Up @@ -1179,35 +1179,38 @@ def execute(
if as_process:
return self.AutoInterrupt(proc, command)

def kill_process(pid: int) -> None:
"""Callback to kill a process."""
if os.name == "nt":
raise AssertionError("Bug: This callback would be ineffective and unsafe on Windows, stopping.")
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
kill_check.set() # Tell the main routine that the process was killed.
except OSError:
# It is possible that the process gets completed in the duration after
# timeout happens and before we try to kill the process.
pass
return

# END kill_process

if kill_after_timeout is not None:
if sys.platform != "win32" and kill_after_timeout is not None:

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

# END kill_process

kill_check = threading.Event()
watchdog = threading.Timer(kill_after_timeout, kill_process, args=(proc.pid,))

Expand All @@ -1218,10 +1221,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 is not None:
if sys.platform != "win32" and kill_after_timeout is not None:
watchdog.start()
stdout_value, stderr_value = proc.communicate()
if kill_after_timeout is not None:
if sys.platform != "win32" and kill_after_timeout is not None:
watchdog.cancel()
if kill_check.is_set():
stderr_value = 'Timeout: the command "%s" did not complete in %d ' "secs." % (
Expand Down

0 comments on commit 4191f7d

Please sign in to comment.