FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Merge pull request #2277 from gitpython-developers/kill-all-proper · gitpython-developers/GitPython@1af7ce6 · GitHub

  1. gitpython-developers
  2. GitPython

Repository navigation

Commit 1af7ce6

Browse files
Byron
authored
Merge pull request #2277 from gitpython-developers/kill-all-proper
fix: stop stalled remote commands when their timeout expires
2 parents f6a043a + 5ba236e commit 1af7ce6

5 files changed

Lines changed: 309 additions & 108 deletions

File tree

‎git/cmd.py‎

Lines changed: 119 additions & 102 deletions
Original file line numberDiff line numberDiff line change
@@ -15,57 +15,57 @@
1515
import re
1616
import signal
1717
import subprocess
18-
from subprocess import DEVNULL, PIPE, Popen
1918
import sys
20-
from textwrap import dedent
2119
import threading
20+
import time
2221
import warnings
23-
24-
from git.compat import defenc, force_bytes, safe_decode
25-
from git.exc import (
26-
CommandError,
27-
GitCommandError,
28-
GitCommandNotFound,
29-
UnsafeOptionError,
30-
UnsafeProtocolError,
31-
)
32-
from git.util import (
33-
cygpath,
34-
expand_path,
35-
is_cygwin_git,
36-
patch_env,
37-
remove_password_if_present,
38-
stream_copy,
39-
)
22+
from subprocess import DEVNULL, PIPE, Popen
23+
from textwrap import dedent
4024

4125
# typing ---------------------------------------------------------------------------
42-
4326
from typing import (
27+
IO,
28+
TYPE_CHECKING,
4429
Any,
4530
AnyStr,
4631
BinaryIO,
4732
Callable,
4833
Dict,
49-
IO,
5034
Iterator,
5135
List,
5236
Mapping,
5337
Optional,
5438
Sequence,
55-
TYPE_CHECKING,
5639
TextIO,
5740
Tuple,
5841
Union,
5942
cast,
6043
overload,
6144
)
6245

46+
from git.compat import defenc, force_bytes, safe_decode
47+
from git.exc import (
48+
CommandError,
49+
GitCommandError,
50+
GitCommandNotFound,
51+
UnsafeOptionError,
52+
UnsafeProtocolError,
53+
)
54+
from git.util import (
55+
cygpath,
56+
expand_path,
57+
is_cygwin_git,
58+
patch_env,
59+
remove_password_if_present,
60+
stream_copy,
61+
)
62+
6363
if sys.version_info >= (3, 10):
6464
from typing import TypeAlias
6565
else:
6666
from typing_extensions import TypeAlias
6767

68-
from git.types import Literal, PathLike, TBD
68+
from git.types import TBD, Literal, PathLike
6969

7070
if TYPE_CHECKING:
7171
from git.diff import DiffIndex
@@ -99,6 +99,45 @@
9999
## @{
100100

101101

102+
def _kill_process(pid: int) -> bool:
103+
"""Kill a POSIX process and its descendants, returning whether it was killed."""
104+
# Collect descendants before signalling, while their parent PIDs still identify them.
105+
pids = [pid]
106+
for parent_pid in pids:
107+
try:
108+
try:
109+
p = Popen(["pgrep", "-P", str(parent_pid)], stdout=PIPE)
110+
except FileNotFoundError:
111+
# POSIX ps does not support selecting by parent PID.
112+
ps_args = ["ps", "-ef"] if sys.platform == "cygwin" else ["ps", "-A", "-o", "pid=", "-o", "ppid="]
113+
with Popen(ps_args, stdout=PIPE) as p:
114+
if p.stdout is not None:
115+
for line in p.stdout:
116+
fields = line.split()
117+
if sys.platform == "cygwin":
118+
fields = fields[1:3] # ps -ef starts with UID, PID, PPID.
119+
if len(fields) == 2 and all(field.isdigit() for field in fields):
120+
if int(fields[1]) == parent_pid:
121+
pids.append(int(fields[0]))
122+
else:
123+
with p:
124+
if p.stdout is not None:
125+
for line in p.stdout:
126+
if line.strip().isdigit():
127+
pids.append(int(line))
128+
except OSError as ex:
129+
_logger.info("Unable to enumerate child processes: %r", ex)
130+
killed = False
131+
if sys.platform != "win32":
132+
for process_pid in pids:
133+
try:
134+
os.kill(process_pid, signal.SIGKILL)
135+
killed = killed or process_pid == pid
136+
except OSError:
137+
pass
138+
return killed
139+
140+
102141
def handle_process_output(
103142
process: Union["Git.AutoInterrupt", Popen],
104143
stdout_handler: Union[
@@ -165,7 +204,7 @@ def pump_stream(
165204

166205
except Exception as ex:
167206
_logger.error(f"Pumping {name!r} of cmd({remove_password_if_present(cmdline)}) failed due to: {ex!r}")
168-
if "I/O operation on closed file" not in str(ex):
207+
if not (isinstance(ex, ValueError) and stream.closed):
169208
# Only reraise if the error was not due to the stream closing.
170209
raise CommandError([f"<{name}-pump>"] + remove_password_if_present(cmdline), ex) from ex
171210
finally:
@@ -199,28 +238,26 @@ def pump_stream(
199238
t.start()
200239
threads.append(t)
201240

202-
# FIXME: Why join? Will block if stdin needs feeding...
241+
# Wait for output handlers to finish before finalizing.
242+
# If the child needs stdin, the caller must arrange to feed/close it
243+
# before this call or concurrently; this function only drains output.
244+
deadline = None if kill_after_timeout is None else time.monotonic() + kill_after_timeout
203245
for t in threads:
204-
t.join(timeout=kill_after_timeout)
246+
t.join(timeout=None if deadline is None else max(0, deadline - time.monotonic()))
205247
if t.is_alive():
206248
if isinstance(process, Git.AutoInterrupt):
249+
if sys.platform != "win32" and process.proc is not None and process.proc.poll() is None:
250+
_kill_process(process.proc.pid)
207251
process._terminate()
208252
else: # Don't want to deal with the other case.
209253
raise RuntimeError(
210254
"Thread join() timed out in cmd.handle_process_output()."
211255
f" kill_after_timeout={kill_after_timeout} seconds"
212256
)
213-
if stderr_handler:
214-
error_str: Union[str, bytes] = (
215-
f"error: process killed because it timed out. kill_after_timeout={kill_after_timeout} seconds"
216-
)
217-
if not decode_streams and isinstance(p_stderr, BinaryIO):
218-
# Assume stderr_handler needs binary input.
219-
error_str = cast(str, error_str)
220-
error_str = error_str.encode()
221-
# We ignore typing on the next line because mypy does not like the way
222-
# we inferred that stderr takes str or bytes.
223-
stderr_handler(error_str) # type: ignore[arg-type]
257+
process._timeout_error = (
258+
f"error: process killed because it timed out. kill_after_timeout={kill_after_timeout} seconds"
259+
)
260+
break
224261

225262
if finalizer:
226263
finalizer(process)
@@ -326,7 +363,7 @@ class _AutoInterrupt:
326363
raise.
327364
"""
328365

329-
__slots__ = ("proc", "args", "status")
366+
__slots__ = ("proc", "args", "status", "_timeout_error")
330367

331368
# If this is non-zero it will override any status code during _terminate, used
332369
# to prevent race conditions in testing.
@@ -336,6 +373,7 @@ def __init__(self, proc: Union[None, subprocess.Popen], args: Any) -> None:
336373
self.proc = proc
337374
self.args = args
338375
self.status: Union[int, None] = None
376+
self._timeout_error: Optional[str] = None
339377

340378
def _terminate(self) -> None:
341379
"""Terminate the underlying process."""
@@ -344,37 +382,45 @@ def _terminate(self) -> None:
344382

345383
proc = self.proc
346384
self.proc = None
347-
if proc.stdin:
348-
proc.stdin.close()
349-
if proc.stdout:
350-
proc.stdout.close()
351-
if proc.stderr:
352-
proc.stderr.close()
353-
# Did the process finish already so we have a return code?
354385
try:
355-
if proc.poll() is not None:
356-
self.status = self._status_code_if_terminate or proc.poll()
386+
if proc.stdin:
387+
# A timed-out process may already have exited before input is flushed.
388+
with contextlib.suppress(BrokenPipeError):
389+
proc.stdin.close()
390+
# Did the process finish already so we have a return code?
391+
try:
392+
if proc.poll() is not None:
393+
self.status = self._status_code_if_terminate or proc.poll()
394+
return
395+
except OSError as ex:
396+
_logger.info("Ignored error after process had died: %r", ex)
397+
398+
# It can be that nothing really exists anymore...
399+
if getattr(os, "kill", None) is None:
357400
return
358-
except OSError as ex:
359-
_logger.info("Ignored error after process had died: %r", ex)
360401

361-
# It can be that nothing really exists anymore...
362-
if os is None or getattr(os, "kill", None) is None:
363-
return
402+
# Try to kill it.
403+
try:
404+
proc.terminate()
405+
except (OSError, AttributeError) as ex:
406+
# On interpreter shutdown (notably on Windows), parts of the stdlib used by
407+
# subprocess can already be torn down (e.g. `subprocess._winapi` becomes None),
408+
# which can cause AttributeError during terminate(). In that case, we prefer
409+
# to silently ignore to avoid noisy "Exception ignored in: __del__" messages.
410+
_logger.info("Ignored error while terminating process: %r", ex)
411+
return
412+
# END exception handling
413+
finally:
414+
if proc.stdout:
415+
proc.stdout.close()
416+
if proc.stderr:
417+
proc.stderr.close()
364418

365-
# Try to kill it.
366419
try:
367-
proc.terminate()
368-
status = proc.wait() # Ensure the process goes away.
369-
420+
status = proc.wait()
370421
self.status = self._status_code_if_terminate or status
371422
except (OSError, AttributeError) as ex:
372-
# On interpreter shutdown (notably on Windows), parts of the stdlib used by
373-
# subprocess can already be torn down (e.g. `subprocess._winapi` becomes None),
374-
# which can cause AttributeError during terminate(). In that case, we prefer
375-
# to silently ignore to avoid noisy "Exception ignored in: __del__" messages.
376-
_logger.info("Ignored error while terminating process: %r", ex)
377-
# END exception handling
423+
_logger.info("Ignored error while waiting for terminated process: %r", ex)
378424

379425
def __del__(self) -> None:
380426
self._terminate()
@@ -393,9 +439,8 @@ def wait(self, stderr: Union[None, str, bytes] = b"") -> int:
393439
May deadlock if output or error pipes are used and not handled separately.
394440
395441
:raise git.exc.GitCommandError:
396-
If the return status is not 0.
442+
If the return status is not 0 or output handling timed out.
397443
"""
398-
stderr_b = force_bytes(data=stderr, encoding="utf-8") or b""
399444
status: Union[int, None]
400445
if self.proc is not None:
401446
status = self.proc.wait()
@@ -404,6 +449,11 @@ def wait(self, stderr: Union[None, str, bytes] = b"") -> int:
404449
status = self.status
405450
p_stderr = None
406451

452+
stderr_b = force_bytes(data=stderr, encoding="utf-8") or b""
453+
if self._timeout_error is not None:
454+
stderr_b += (b"\n" if stderr_b else b"") + self._timeout_error.encode("utf-8")
455+
status = status or 1
456+
407457
def read_all_from_possibly_closed_stream(stream: Union[IO[bytes], None]) -> bytes:
408458
if stream:
409459
try:
@@ -1381,9 +1431,10 @@ def execute(
13811431
1. This feature is not supported at all on Windows.
13821432
2. Enumerating child processes requires ``pgrep -P``, or a ``ps`` command
13831433
supporting the POSIX ``-A`` and ``-o`` options if ``pgrep`` is not
1384-
installed. Effectiveness may vary on systems without these commands.
1385-
3. Deeper descendants do not receive signals, though they may sometimes
1386-
terminate as a consequence of their parent processes being killed.
1434+
installed (``ps -ef`` on Cygwin). Effectiveness may vary on systems
1435+
without these commands.
1436+
3. Descendants are enumerated before signalling. Processes that detach
1437+
or spawn after enumeration may not receive signals.
13871438
4. `kill_after_timeout` uses ``SIGKILL``, which can have negative side
13881439
effects on a repository. For example, stale locks in case of
13891440
:manpage:`git-gc(1)` could render the repository incapable of accepting
@@ -1537,43 +1588,9 @@ def execute(
15371588
timeout = kill_after_timeout
15381589

15391590
def kill_process(pid: int) -> None:
1540-
"""Callback to kill a process.
1541-
1542-
This callback implementation would be ineffective and unsafe on Windows.
1543-
"""
1544-
child_pids = []
1545-
try:
1546-
p = Popen(["pgrep", "-P", str(pid)], stdout=PIPE)
1547-
except FileNotFoundError:
1548-
# POSIX ps does not support selecting by parent PID.
1549-
with Popen(["ps", "-A", "-o", "pid=", "-o", "ppid="], stdout=PIPE) as p:
1550-
if p.stdout is not None:
1551-
for line in p.stdout:
1552-
fields = line.split()
1553-
if len(fields) == 2 and all(field.isdigit() for field in fields):
1554-
if int(fields[1]) == pid:
1555-
child_pids.append(int(fields[0]))
1556-
else:
1557-
with p:
1558-
if p.stdout is not None:
1559-
for line in p.stdout:
1560-
if line.strip().isdigit():
1561-
child_pids.append(int(line))
1562-
try:
1563-
os.kill(pid, signal.SIGKILL)
1564-
for child_pid in child_pids:
1565-
try:
1566-
os.kill(child_pid, signal.SIGKILL)
1567-
except OSError:
1568-
pass
1569-
# Tell the main routine that the process was killed.
1591+
if _kill_process(pid):
15701592
assert kill_check is not None
15711593
kill_check.set()
1572-
except OSError:
1573-
# It is possible that the process gets completed in the duration
1574-
# after timeout happens and before we try to kill the process.
1575-
pass
1576-
return
15771594

15781595
def make_timeout_error() -> Union[str, bytes]:
15791596
err = f'Timeout: the command "{" ".join(redacted_command)}" did not complete in {timeout:g} secs.'

‎git/remote.py‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -988,8 +988,9 @@ def stdout_handler(line: str) -> None:
988988
# even if there is an output).
989989
if not output:
990990
raise
991-
elif stderr_text:
992-
_logger.warning("Error lines received while fetching: %s", stderr_text)
991+
elif stderr_text or proc._timeout_error:
992+
if stderr_text:
993+
_logger.warning("Error lines received while pushing: %s", stderr_text)
993994
output.error = e
994995

995996
return output

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL