Skip to content

Commit 4191f7d

Browse files
committed
Refactor kill_after_timeout logic so mypy can check it
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 (#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.)
1 parent dc95a76 commit 4191f7d

File tree

1 file changed

+35
-32
lines changed

1 file changed

+35
-32
lines changed

Diff for: git/cmd.py

+35-32
Original file line numberDiff line numberDiff line change
@@ -1134,7 +1134,7 @@ def execute(
11341134
if inline_env is not None:
11351135
env.update(inline_env)
11361136

1137-
if os.name == "nt":
1137+
if sys.platform == "win32":
11381138
cmd_not_found_exception = OSError
11391139
if kill_after_timeout is not None:
11401140
raise GitCommandError(
@@ -1179,35 +1179,38 @@ def execute(
11791179
if as_process:
11801180
return self.AutoInterrupt(proc, command)
11811181

1182-
def kill_process(pid: int) -> None:
1183-
"""Callback to kill a process."""
1184-
if os.name == "nt":
1185-
raise AssertionError("Bug: This callback would be ineffective and unsafe on Windows, stopping.")
1186-
p = Popen(["ps", "--ppid", str(pid)], stdout=PIPE)
1187-
child_pids = []
1188-
if p.stdout is not None:
1189-
for line in p.stdout:
1190-
if len(line.split()) > 0:
1191-
local_pid = (line.split())[0]
1192-
if local_pid.isdigit():
1193-
child_pids.append(int(local_pid))
1194-
try:
1195-
os.kill(pid, signal.SIGKILL)
1196-
for child_pid in child_pids:
1197-
try:
1198-
os.kill(child_pid, signal.SIGKILL)
1199-
except OSError:
1200-
pass
1201-
kill_check.set() # Tell the main routine that the process was killed.
1202-
except OSError:
1203-
# It is possible that the process gets completed in the duration after
1204-
# timeout happens and before we try to kill the process.
1205-
pass
1206-
return
1207-
1208-
# END kill_process
1209-
1210-
if kill_after_timeout is not None:
1182+
if sys.platform != "win32" and kill_after_timeout is not None:
1183+
1184+
def kill_process(pid: int) -> None:
1185+
"""Callback to kill a process.
1186+
1187+
This callback implementation would be ineffective and unsafe on Windows.
1188+
"""
1189+
p = Popen(["ps", "--ppid", str(pid)], stdout=PIPE)
1190+
child_pids = []
1191+
if p.stdout is not None:
1192+
for line in p.stdout:
1193+
if len(line.split()) > 0:
1194+
local_pid = (line.split())[0]
1195+
if local_pid.isdigit():
1196+
child_pids.append(int(local_pid))
1197+
try:
1198+
os.kill(pid, signal.SIGKILL)
1199+
for child_pid in child_pids:
1200+
try:
1201+
os.kill(child_pid, signal.SIGKILL)
1202+
except OSError:
1203+
pass
1204+
# Tell the main routine that the process was killed.
1205+
kill_check.set()
1206+
except OSError:
1207+
# It is possible that the process gets completed in the duration
1208+
# after timeout happens and before we try to kill the process.
1209+
pass
1210+
return
1211+
1212+
# END kill_process
1213+
12111214
kill_check = threading.Event()
12121215
watchdog = threading.Timer(kill_after_timeout, kill_process, args=(proc.pid,))
12131216

@@ -1218,10 +1221,10 @@ def kill_process(pid: int) -> None:
12181221
newline = "\n" if universal_newlines else b"\n"
12191222
try:
12201223
if output_stream is None:
1221-
if kill_after_timeout is not None:
1224+
if sys.platform != "win32" and kill_after_timeout is not None:
12221225
watchdog.start()
12231226
stdout_value, stderr_value = proc.communicate()
1224-
if kill_after_timeout is not None:
1227+
if sys.platform != "win32" and kill_after_timeout is not None:
12251228
watchdog.cancel()
12261229
if kill_check.is_set():
12271230
stderr_value = 'Timeout: the command "%s" did not complete in %d ' "secs." % (

0 commit comments

Comments
 (0)