|
19 | 19 | import sys |
20 | 20 | from textwrap import dedent |
21 | 21 | import threading |
| 22 | +import time |
22 | 23 | import warnings |
23 | 24 |
|
24 | 25 | from git.compat import defenc, force_bytes, safe_decode |
|
99 | 100 | ## @{ |
100 | 101 |
|
101 | 102 |
|
| 103 | +def _kill_process(pid: int) -> bool: |
| 104 | + """Kill a POSIX process and its descendants, returning whether it was killed.""" |
| 105 | + # Collect descendants before signalling, while their parent PIDs still identify them. |
| 106 | + pids = [pid] |
| 107 | + for parent_pid in pids: |
| 108 | + try: |
| 109 | + try: |
| 110 | + p = Popen(["pgrep", "-P", str(parent_pid)], stdout=PIPE) |
| 111 | + except FileNotFoundError: |
| 112 | + # POSIX ps does not support selecting by parent PID. |
| 113 | + with Popen(["ps", "-A", "-o", "pid=", "-o", "ppid="], stdout=PIPE) as p: |
| 114 | + if p.stdout is not None: |
| 115 | + for line in p.stdout: |
| 116 | + fields = line.split() |
| 117 | + if len(fields) == 2 and all(field.isdigit() for field in fields): |
| 118 | + if int(fields[1]) == parent_pid: |
| 119 | + pids.append(int(fields[0])) |
| 120 | + else: |
| 121 | + with p: |
| 122 | + if p.stdout is not None: |
| 123 | + for line in p.stdout: |
| 124 | + if line.strip().isdigit(): |
| 125 | + pids.append(int(line)) |
| 126 | + except OSError as ex: |
| 127 | + _logger.info("Unable to enumerate child processes: %r", ex) |
| 128 | + killed = False |
| 129 | + if sys.platform != "win32": |
| 130 | + for process_pid in pids: |
| 131 | + try: |
| 132 | + os.kill(process_pid, signal.SIGKILL) |
| 133 | + killed = killed or process_pid == pid |
| 134 | + except OSError: |
| 135 | + pass |
| 136 | + return killed |
| 137 | + |
| 138 | + |
102 | 139 | def handle_process_output( |
103 | 140 | process: Union["Git.AutoInterrupt", Popen], |
104 | 141 | stdout_handler: Union[ |
@@ -165,7 +202,7 @@ def pump_stream( |
165 | 202 |
|
166 | 203 | except Exception as ex: |
167 | 204 | _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): |
| 205 | + if not (isinstance(ex, ValueError) and stream.closed): |
169 | 206 | # Only reraise if the error was not due to the stream closing. |
170 | 207 | raise CommandError([f"<{name}-pump>"] + remove_password_if_present(cmdline), ex) from ex |
171 | 208 | finally: |
@@ -200,27 +237,23 @@ def pump_stream( |
200 | 237 | threads.append(t) |
201 | 238 |
|
202 | 239 | # FIXME: Why join? Will block if stdin needs feeding... |
| 240 | + deadline = None if kill_after_timeout is None else time.monotonic() + kill_after_timeout |
203 | 241 | for t in threads: |
204 | | - t.join(timeout=kill_after_timeout) |
| 242 | + t.join(timeout=None if deadline is None else max(0, deadline - time.monotonic())) |
205 | 243 | if t.is_alive(): |
206 | 244 | if isinstance(process, Git.AutoInterrupt): |
| 245 | + if sys.platform != "win32" and process.proc is not None: |
| 246 | + _kill_process(process.proc.pid) |
207 | 247 | process._terminate() |
208 | 248 | else: # Don't want to deal with the other case. |
209 | 249 | raise RuntimeError( |
210 | 250 | "Thread join() timed out in cmd.handle_process_output()." |
211 | 251 | f" kill_after_timeout={kill_after_timeout} seconds" |
212 | 252 | ) |
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] |
| 253 | + process._timeout_error = ( |
| 254 | + f"error: process killed because it timed out. kill_after_timeout={kill_after_timeout} seconds" |
| 255 | + ) |
| 256 | + break |
224 | 257 |
|
225 | 258 | if finalizer: |
226 | 259 | finalizer(process) |
@@ -326,7 +359,7 @@ class _AutoInterrupt: |
326 | 359 | raise. |
327 | 360 | """ |
328 | 361 |
|
329 | | - __slots__ = ("proc", "args", "status") |
| 362 | + __slots__ = ("proc", "args", "status", "_timeout_error") |
330 | 363 |
|
331 | 364 | # If this is non-zero it will override any status code during _terminate, used |
332 | 365 | # to prevent race conditions in testing. |
@@ -336,6 +369,7 @@ def __init__(self, proc: Union[None, subprocess.Popen], args: Any) -> None: |
336 | 369 | self.proc = proc |
337 | 370 | self.args = args |
338 | 371 | self.status: Union[int, None] = None |
| 372 | + self._timeout_error: Optional[str] = None |
339 | 373 |
|
340 | 374 | def _terminate(self) -> None: |
341 | 375 | """Terminate the underlying process.""" |
@@ -344,37 +378,45 @@ def _terminate(self) -> None: |
344 | 378 |
|
345 | 379 | proc = self.proc |
346 | 380 | 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? |
354 | 381 | try: |
355 | | - if proc.poll() is not None: |
356 | | - self.status = self._status_code_if_terminate or proc.poll() |
| 382 | + if proc.stdin: |
| 383 | + # A timed-out process may already have exited before input is flushed. |
| 384 | + with contextlib.suppress(BrokenPipeError): |
| 385 | + proc.stdin.close() |
| 386 | + # Did the process finish already so we have a return code? |
| 387 | + try: |
| 388 | + if proc.poll() is not None: |
| 389 | + self.status = self._status_code_if_terminate or proc.poll() |
| 390 | + return |
| 391 | + except OSError as ex: |
| 392 | + _logger.info("Ignored error after process had died: %r", ex) |
| 393 | + |
| 394 | + # It can be that nothing really exists anymore... |
| 395 | + if os is None or getattr(os, "kill", None) is None: |
357 | 396 | return |
358 | | - except OSError as ex: |
359 | | - _logger.info("Ignored error after process had died: %r", ex) |
360 | 397 |
|
361 | | - # It can be that nothing really exists anymore... |
362 | | - if os is None or getattr(os, "kill", None) is None: |
363 | | - return |
| 398 | + # Try to kill it. |
| 399 | + try: |
| 400 | + proc.terminate() |
| 401 | + except (OSError, AttributeError) as ex: |
| 402 | + # On interpreter shutdown (notably on Windows), parts of the stdlib used by |
| 403 | + # subprocess can already be torn down (e.g. `subprocess._winapi` becomes None), |
| 404 | + # which can cause AttributeError during terminate(). In that case, we prefer |
| 405 | + # to silently ignore to avoid noisy "Exception ignored in: __del__" messages. |
| 406 | + _logger.info("Ignored error while terminating process: %r", ex) |
| 407 | + return |
| 408 | + # END exception handling |
| 409 | + finally: |
| 410 | + if proc.stdout: |
| 411 | + proc.stdout.close() |
| 412 | + if proc.stderr: |
| 413 | + proc.stderr.close() |
364 | 414 |
|
365 | | - # Try to kill it. |
366 | 415 | try: |
367 | | - proc.terminate() |
368 | | - status = proc.wait() # Ensure the process goes away. |
369 | | - |
| 416 | + status = proc.wait() |
370 | 417 | self.status = self._status_code_if_terminate or status |
371 | 418 | 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 |
| 419 | + _logger.info("Ignored error while waiting for terminated process: %r", ex) |
378 | 420 |
|
379 | 421 | def __del__(self) -> None: |
380 | 422 | self._terminate() |
@@ -395,6 +437,8 @@ def wait(self, stderr: Union[None, str, bytes] = b"") -> int: |
395 | 437 | :raise git.exc.GitCommandError: |
396 | 438 | If the return status is not 0. |
397 | 439 | """ |
| 440 | + if self._timeout_error is not None: |
| 441 | + stderr = self._timeout_error |
398 | 442 | stderr_b = force_bytes(data=stderr, encoding="utf-8") or b"" |
399 | 443 | status: Union[int, None] |
400 | 444 | if self.proc is not None: |
@@ -1382,8 +1426,8 @@ def execute( |
1382 | 1426 | 2. Enumerating child processes requires ``pgrep -P``, or a ``ps`` command |
1383 | 1427 | supporting the POSIX ``-A`` and ``-o`` options if ``pgrep`` is not |
1384 | 1428 | 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. |
| 1429 | + 3. Descendants are enumerated before signalling. Processes that detach |
| 1430 | + or spawn after enumeration may not receive signals. |
1387 | 1431 | 4. `kill_after_timeout` uses ``SIGKILL``, which can have negative side |
1388 | 1432 | effects on a repository. For example, stale locks in case of |
1389 | 1433 | :manpage:`git-gc(1)` could render the repository incapable of accepting |
@@ -1537,43 +1581,9 @@ def execute( |
1537 | 1581 | timeout = kill_after_timeout |
1538 | 1582 |
|
1539 | 1583 | 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. |
| 1584 | + if _kill_process(pid): |
1570 | 1585 | assert kill_check is not None |
1571 | 1586 | 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 |
1577 | 1587 |
|
1578 | 1588 | def make_timeout_error() -> Union[str, bytes]: |
1579 | 1589 | err = f'Timeout: the command "{" ".join(redacted_command)}" did not complete in {timeout:g} secs.' |
|