add a pidfd child process watcher

[benjaminp]

BPO	38692
Nosy	@asvetlov, @1st1
PRs	bpo-38692: Add os.pidfd_open. #17063 closes bpo-38692: Add a pidfd child process watcher to asyncio. #17069 bpo-38692: Add asyncio.PidfdChildWatcher to __all__ #17161 bpo-38692: Skip test_posix.test_pidfd_open() on EPERM #17290 bpo-38692: Add audit events to pidfd related functions #25515
Files	0001-asyncio-Add-a-pidfd-child-process-watcher.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2019-11-21.11:56:41.300>
created_at = <Date 2019-11-05.05:43:04.618>
labels = ['type-feature', '3.9', 'expert-asyncio']
title = 'add a pidfd child process watcher'
updated_at = <Date 2021-11-04.14:23:56.816>
user = 'https://github.com/benjaminp'

bugs.python.org fields:

activity = <Date 2021-11-04.14:23:56.816>
actor = 'eryksun'
assignee = 'none'
closed = True
closed_date = <Date 2019-11-21.11:56:41.300>
closer = 'vstinner'
components = ['asyncio']
creation = <Date 2019-11-05.05:43:04.618>
creator = 'benjamin.peterson'
dependencies = []
files = ['48695']
hgrepos = []
issue_num = 38692
keywords = ['patch']
message_count = 45.0
messages = ['356001', '356016', '356017', '356024', '356026', '356047', '356048', '356068', '356069', '356071', '356079', '356087', '356148', '356164', '356235', '356251', '356286', '356287', '356288', '356289', '356292', '356488', '356491', '356492', '356493', '356494', '356497', '356556', '356566', '356567', '356569', '356570', '356595', '356642', '356643', '357054', '357056', '357057', '357058', '357076', '357077', '357162', '357163', '357271', '357272']
nosy_count = 2.0
nosy_names = ['asvetlov', 'yselivanov']
pr_nums = ['17063', '17069', '17161', '17290', '25515']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue38692'
versions = ['Python 3.9']

[benjaminp]

Recent versions of Linux has built out support for pidfd, a way to do process management with file descriptors. I wanted to try it out, so implemented a pidfd-based child watcher for asyncio.

My WIP progress patch is attached. It passes all asyncio tests.

[asvetlov]

Awesome!

I think the patch can be splitted in os.pidfd_open() and asyncio part itself.

os modification is clear.

asyncio part looks correct after the brief view.
My the main question is: how to detect if the new watcher can be used or asyncio should fallback to threaded based solution?

[asvetlov]

Nathaniel, you may be interested in the pidfd_open(), at least in adding the function to os module.

[vstinner]

I like the idea of exposing pidfd_open() syscall as os.pidfd_open().

Benjamin: Would you mind to create a PR based on your patch, but without your asyncio change?

+#ifndef __NR_pidfd_open

I would prefer to avoid this part of the patch.

You should accept an optional flags parameter, no?

[vstinner]

+ self._loop._remove_reader(pidfd)
+ os.close(pidfd)

Note: Maybe do these operations in the reverse order. I expect a higher risk of exception when calling Python self._loop._remove_reader(), than on closing a FD that we opened.

[njsmith]

Thanks for the CC.

It would be nice to get pidfd_send_signal as well, while we're at it. But that could be a separate PR.

AFAICT the other bits of the pidfd API right now are some extra flags to clone, and an extra flag to waitid. The waitid flag is nice-to-have but not really urgent, since it's easy enough to use a flag even if the stdlib doesn't explicitly expose it. The clone flags are super interesting, but before we can use them, we need to have some way to access clone itself, and that's a big complicated project, so we definitely shouldn't worry about it here.

[benjaminp]

Yes, I will be submitting followup changes for pidfd_send_signal and the other goodies.

I would like to use pidfds in subprocess, but as you as you say, that's another kettle of fish.

[vstinner]

See also bpo-38630 "subprocess.Popen.send_signal() should poll the process".

[aeros]

My the main question is: how to detect if the new watcher can be used or asyncio should fallback to threaded based solution?

Perhaps in the __init__ we could do something like this:

class PidfdChildWatcher(AbstractChildWatcher):

    def __init__(self):
        if not hasattr(os, "pidfd_open"):
             raise RuntimeError("os.pidfd_open() is unavailable.")
        ...

Thoughts?

My WIP progress patch is attached. It passes all asyncio tests.

I think we could also specifically look for race conditions with ./python -m test test_asyncio.test_subprocess -F -j4, rather than relying on the tests passing with a single job. The other child watchers have been particularly infamous when it comes race conditions and resource leaks.

I'd be glad to work on testing and opening a PR to asyncio if needed (while giving Benjamin credit for the patch of course).

[njsmith]

hasattr is useful for supporting old versions of the os module, but asyncio doesn't have to care about that. You should probably try calling pidfd_open and see whether you get -ESYSCALL, and also maybe try passing it to poll(), since I think there might have been a release where the syscall existed but it wasn't pollable. Not sure what the error for that looks like.

[benjaminp]

pidfd_open was added after pidfd pollling, so it should suffice to make sure pidfd_open doesn't ENOSYS.

[benjaminp]

New changeset 6c4c45e by Benjamin Peterson in branch 'master':
bpo-38692: Add os.pidfd_open. (GH-17063)
6c4c45e