Skip to content

Commit c640edd

Browse files
transports: use a thread instead of SafeChildWatcher
SafeChildWatcher was removed in 3.14. We used to use this as a fallback for when we don't have pidfd support in the kernel (or Python stdlib) under the assumption that we'd never see Python 3.14 on a system with such an old kernel, but of course this happens with newer Python versions in containers running on RHEL 8 hosts. Let's divorce ourselves from the entire concept of ChildWatchers, dropping our 'qdata' cache for them. Instead, we just try to create a pidfd each time, and if that fails, start up a thread to call waitpid() and notify the event loop via call_soon_threadsafe(). This approach is a simplified form of asyncio.ThreadedChildWatcher from the standard library, which is what gets used anywhere pidfds aren't available. We had a test that caused pidfd to return ENOSYS that we started skipping on newer Python versions to avoid trying to use SafeChildWatcher in what I imagined to be an impossible configuration. Bring that back unconditionally (and rename it to remove the reference to "safe watcher").
1 parent 8b39e7a commit c640edd

File tree

2 files changed

+21
-39
lines changed

2 files changed

+21
-39
lines changed

src/cockpit/transports.py

Lines changed: 20 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import struct
1818
import subprocess
1919
import termios
20+
from threading import Thread
2021
from typing import Any, ClassVar, Sequence
2122

2223
from .jsonutil import JsonObject, get_int
@@ -33,6 +34,8 @@ def prctl(*args: int) -> None:
3334

3435

3536
logger = logging.getLogger(__name__)
37+
38+
3639
IOV_MAX = 1024 # man 2 writev
3740

3841

@@ -305,50 +308,35 @@ def get_stderr(self, *, reset: bool = False) -> str:
305308
return ''
306309

307310
def watch_exit(self, process: 'subprocess.Popen[bytes]') -> None:
308-
def flag_exit() -> None:
311+
def child_exited(pid: int, status: int) -> None:
312+
assert pid == process.pid
313+
try:
314+
self._returncode = os.waitstatus_to_exitcode(status)
315+
except ValueError:
316+
self._returncode = status
317+
309318
assert isinstance(self._protocol, SubprocessProtocol)
310319
logger.debug('Process exited with status %d', self._returncode)
311320
if not self._closing:
312321
self._protocol.process_exited()
313322

314323
def pidfd_ready() -> None:
315-
pid, status = os.waitpid(process.pid, 0)
316-
assert pid == process.pid
317-
try:
318-
self._returncode = os.waitstatus_to_exitcode(status)
319-
except ValueError:
320-
self._returncode = status
324+
pid, status = os.waitpid(process.pid, 0) # should never block
321325
self._loop.remove_reader(pidfd)
322326
os.close(pidfd)
323-
flag_exit()
324-
325-
def child_watch_fired(pid: int, code: int) -> None:
326-
assert process.pid == pid
327-
self._returncode = code
328-
flag_exit()
329-
330-
# We first try to create a pidfd to track the process manually. If
331-
# that does work, we need to create a SafeChildWatcher, which has been
332-
# deprecated and removed in Python 3.14. This effectively means that
333-
# using Python 3.14 requires that we're running on a kernel with pidfd
334-
# support, which is fine: the only place we still care about such old
335-
# kernels is on RHEL8 and we have Python 3.6 there.
327+
child_exited(pid, status)
328+
329+
def waitpid_thread() -> None:
330+
pid, status = os.waitpid(process.pid, 0) # will block
331+
self._loop.call_soon_threadsafe(child_exited, pid, status)
332+
333+
# We first try to create a pidfd to track the process. If that doesn't
334+
# work, we spawn a thread to do a blocking waitpid().
336335
try:
337336
pidfd = os.pidfd_open(process.pid)
338337
self._loop.add_reader(pidfd, pidfd_ready)
339338
except (AttributeError, OSError):
340-
quark = '_cockpit_transports_child_watcher'
341-
watcher = getattr(self._loop, quark, None)
342-
343-
if watcher is None:
344-
try:
345-
watcher = asyncio.SafeChildWatcher() # type: ignore[attr-defined]
346-
except AttributeError as e:
347-
raise RuntimeError('pidfd support required on Python 3.14+') from e
348-
watcher.attach_loop(self._loop)
349-
setattr(self._loop, quark, watcher)
350-
351-
watcher.add_child_handler(process.pid, child_watch_fired)
339+
Thread(name=f'cockpit-waitpid-{process.pid}', target=waitpid_thread, daemon=True).start()
352340

353341
def __init__(self,
354342
loop: asyncio.AbstractEventLoop,

test/pytest/test_transport.py

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99
import os
1010
import signal
1111
import subprocess
12-
import sys
1312
import unittest.mock
1413
from typing import Any, List, Optional, Tuple
1514

@@ -289,13 +288,8 @@ async def test_stderr(self) -> None:
289288
assert transport.get_stderr(reset=True) == ''
290289

291290
@pytest.mark.asyncio
292-
async def test_safe_watcher_ENOSYS(self, monkeypatch: pytest.MonkeyPatch) -> None:
291+
async def test_pidfd_ENOSYS(self, monkeypatch: pytest.MonkeyPatch) -> None:
293292
# this test disables pidfd support in order to force the fallback path
294-
# which creates a SafeChildWatcher. That's deprecated since 3.12 and
295-
# removed in 3.14, so skip this test on those versions to avoid issues.
296-
if sys.version_info >= (3, 12, 0):
297-
pytest.skip()
298-
299293
monkeypatch.setattr(os, 'pidfd_open', unittest.mock.Mock(side_effect=OSError), raising=False)
300294
protocol, _transport = self.subprocess(['true'])
301295
await protocol.eof_and_exited_with_code(0)

0 commit comments

Comments
 (0)