transports: use a thread instead of SafeChildWatcher#22913
Merged
allisonkarlitskaya merged 2 commits intocockpit-project:mainfrom Feb 24, 2026
Merged
transports: use a thread instead of SafeChildWatcher#22913allisonkarlitskaya merged 2 commits intocockpit-project:mainfrom
allisonkarlitskaya merged 2 commits intocockpit-project:mainfrom
Conversation
martinpitt
previously approved these changes
Feb 23, 2026
Member
martinpitt
left a comment
martinpitt
left a comment
There was a problem hiding this comment.
Thanks! Given the circumstances of this moving target (asyncio), this is actually a nice solution. 💯
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").
Add tests to verify that non-zero exit codes and signal termination are correctly reported when using the threaded waitpid() fallback path (when pidfd is unavailable). Includes a concurrent test that starts multiple processes, terminates them in a controlled order, and verifies that the correct exit status is attributed to each process, and at the expected time. Assisted-by: Claude Opus 4.5 <noreply@anthropic.com>
allisonkarlitskaya
force-pushed
the
threaded-watcher
branch
from
c640edd to
60edf90
Compare
Member
Author
|
Python 3.6 pain made obscure by the errors of the bridge on the host being sent over ssh to the container and discarded. We really ought to fix the logging story one of these days... |
martinpitt
reviewed
Feb 24, 2026
Member
martinpitt
left a comment
martinpitt
left a comment
There was a problem hiding this comment.
Thanks! Let's do one round of review, but I agree this is urgent.
| def flag_exit() -> None: | ||
| def child_exited(pid: int, status: int) -> None: | ||
| assert pid == process.pid | ||
| # os.waitstatus_to_exitcode() is only available since Python 3.9 |
Member
There was a problem hiding this comment.
That could become a polyfill, for easier future clean-up.
Member
Author
There was a problem hiding this comment.
True. This would fit nicely in polyfills.py with the rest of them.
martinpitt
approved these changes
Feb 24, 2026
Member
Author
|
Agreed to do the polyfill of |
allisonkarlitskaya
merged commit 7157811
into
cockpit-project:main
93 of 94 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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").