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.

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?

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

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?

+        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.

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.

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.

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

> 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).

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.

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

New changeset 6c4c45efaeb40f4f837570f57d90a0b3429c6ae9 by Benjamin Peterson in branch 'master':
bpo-38692: Add os.pidfd_open. (GH-17063)
https://github.com/python/cpython/commit/6c4c45efaeb40f4f837570f57d90a0b3429c6ae9

Would it be useful to use a pidfd in subprocess.Popen to fix bpo-38630 root issue, when pidfd is available?

On Wed, Nov 6, 2019, at 09:57, STINNER Victor wrote:
> 
> STINNER Victor <vstinner@python.org> added the comment:
> 
> Would it be useful to use a pidfd in subprocess.Popen to fix bpo-38630 
> root issue, when pidfd is available?

Probably, but as noted above, we need to figure out the best way to integrate pidfds into subprocess. (Probably not in this issue.)

I got a failure in newly added test_pidfd_open:

======================================================================
FAIL: test_pidfd_open (test.test_posix.PosixTester)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/build/python-git/src/cpython/Lib/test/test_posix.py", line 1479, in test_pidfd_open
    self.assertEqual(cm.exception.errno, errno.EINVAL)
AssertionError: 1 != 22

----------------------------------------------------------------------

I'm running kernel 5.3.7-x86_64-linode130 with Arch Linux. At first I suspect that it's related to system call filters from systemd as tests are run in a systemd-nspawn container. However, there are still failures after patching systemd with https://github.com/systemd/systemd/commit/9e486265716963439fb0fd7f2a97abf109f24f75.

How about also skipping the test if pidfd_open returns EPERM?

I think you must still be experiencing some sort of sandboxing. I don't know how else you would get an EPERM out of pidfd_open.

> I got a failure in newly added test_pidfd_open:

> I'm running kernel 5.3.7-x86_64-linode130 with Arch Linux.

> I think you must still be experiencing some sort of sandboxing. I don't know how else you would get an EPERM out of pidfd_open.

I believe Benjamin is correct. On a native install of Arch Linux with kernel 5.3.7 (using latest updates from the official repos), I received no failures in test_posix. 

[aeros:~/repos/benjaminp-cpython]$ ./python -m test test_posix     (asyncio-pidfd) 
0:00:00 load avg: 1.86 Run tests sequentially
0:00:00 load avg: 1.86 [1/1] test_posix

== Tests result: SUCCESS ==

1 test OK.

Total duration: 544 ms
Tests result: SUCCESS

To confirm there weren't intermittent failures, I also ran test_posix indefinitely, sending SIGINT after ~2500 iterations. No failures occurred:

[aeros:~/repos/benjaminp-cpython]$ ./python -m test test_pty -F    (asyncio-pidfd) 
...
0:01:31 load avg: 1.57 [2506] test_pty
0:01:31 load avg: 1.57 [2507] test_pty
^C

== Tests result: INTERRUPTED ==
Test suite interrupted by signal SIGINT.

2506 tests OK.

Total duration: 1 min 31 sec
Tests result: INTERRUPTED

It seems that the issue is likely specific to Chih-Hsuan Yen's environment.

> [aeros:~/repos/benjaminp-cpython]$ ./python -m test test_pty -F    (asyncio-pidfd) 
...
0:01:31 load avg: 1.57 [2506] test_pty
0:01:31 load avg: 1.57 [2507] test_pty

Oops, looks like I copied the wrong results of a separate test I was running earlier. I'll post the results of ~1000 iterations of test_posix below, once it is completed again.

[aeros:~/repos/benjaminp-cpython]$ ./python -m test test_posix -F  (asyncio-pidfd) 
...
0:08:52 load avg: 1.89 [1008] test_posix
0:08:52 load avg: 2.22 [1009] test_posix
...

1008 tests OK.

Total duration: 8 min 52 sec
Tests result: INTERRUPTED

It seems like systemd-nspawn is just breaking everything: https://sourceware.org/ml/libc-alpha/2019-11/msg00277.html

Benjamin Peterson, Kyle Stanley: Thank you both very much for intensive testing and helpful information! Yes on my Arch Linux box tests are also fine without systemd-nspawn. However, as I have to use systemd-nspawn for some reasons, and investigating how it blocks system calls is beyond my ability, I just add a workaround for now: https://aur.archlinux.org/cgit/aur.git/tree/python-git-issue38692.diff?h=python-git.

> It seems like systemd-nspawn is just breaking everything: https://sourceware.org/ml/libc-alpha/2019-11/msg00277.html

Well, we can try to argue to not block syscalls, but I'm not sure that we can win such battle :-) For os.urandom(), I chose to handle EPERM as ENOSYS in bpo-27955. Extract of Python/bootstrap_hash.c:

            /* ENOSYS: the syscall is not supported by the kernel.
               EPERM: the syscall is blocked by a security policy (ex: SECCOMP)
               or something else. */
            if (errno == ENOSYS || errno == EPERM) {
                getrandom_works = 0;
                return 0;
            }

We can just skip the test if the syscall fails with EPERM.

We should not claim to support running our tests in weird syscall sandboxes. There's an infinite number of possible sandboxing configurations, and we can't fix them all.

> We should not claim to support running our tests in weird syscall sandboxes. There's an infinite number of possible sandboxing configurations, and we can't fix them all.

There is no request to support an "an infinite number of possible sandboxing configurations". Only to skip the test if the syscall fails with EPERM. That sounds reasonable to me.

Sure, that change on it's own looks small and harmless. My point is that it's a slippery slope. Why is that sandbox configuration important enough to handle? It won't be tested by our CI and no one will know whether they can ever remove the EPERM skipping case.

> Why is that sandbox configuration important enough to handle? It won't be tested by our CI and no one will know whether they can ever remove the EPERM skipping case.

It's just convenient for users who use/test Python in a Linux sandbox.

The fact is that 2 days after you merged the new test, a first user reported a failure. I expect more issues if we don't simply fix the test :-) Sandboxes on Linux are more and more common.

It will be fixed, though, as soon as the user upgrades systemd.

We can merge this PR as is (Benjamin, thanks for working on this!), but I think that as soon as we merge it we should do some refactoring and deprecations.

The child watchers API has to go. It's confusing, painful to use, it's not compatible with third-party event loops. It increases the API surface without providing us with enough benefits.

What I propose:

* merge this watcher and try to use it as the default on modern Linuxes.  We don't document it.

* deprecate add_child_watcher and all child watcher classes in 3.9. Aim for removal in 3.11.  

In 3.9 we will use kqueue / pidfd / threads / winapis -- whatever is available, but we never use SIGCHLD by default.

> We can merge this PR as is (Benjamin, thanks for working on this!), but I think that as soon as we merge it we should do some refactoring and deprecations.

> The child watchers API has to go. It's confusing, painful to use, it's not compatible with third-party event loops. It increases the API surface without providing us with enough benefits.

+1

> In 3.9 we will use kqueue / pidfd / threads / winapis -- whatever is available, but we never use SIGCHLD by default.

IIRC, we don't use SIGCHLD by default at the moment, since ThreadedChildWatcher is the default child watcher. Should we change the default to be the new PidfdChildWatcher for Linux kernels 5.3+ in Python 3.9, and fallback to ThreadedChildWatcher for systems without pidfd_open()?

> > The child watchers API has to go. It's confusing, painful to use, it's not compatible with third-party event loops. It increases the API surface without providing us with enough benefits.

> +1

Also, adding to this, the child watchers are one of the least used components of asyncio's public API. So, I think the deprecation and removal cost will be fairly minimal. See the GitHub code usage (includes exact copies of Lib/asyncio/unix_events.py, so there's some redundancy):

MultiLoopChildWatcher: https://github.com/search?l=Python&q=MultiLoopChildWatcher&type=Code (20 results, just added in 3.8)
ThreadedChildWatcher: https://github.com/search?l=Python&q=ThreadedChildWatcher&type=Code (77 results, default unix child watcher, rarely used explicitly)
FastChildWatcher: https://github.com/search?l=Python&q=FastChildWatcher&type=Code (4,426 results)
SafeChildWatcher: https://github.com/search?l=Python&q=SafeChildWatcher&type=Code (7,007 results)
All of asyncio usage: https://github.com/search?l=Python&q=asyncio&type=Code (599,131 results)

Thanks everyone for the reviews. I will go ahead and merge the PR.

Shall we return to #38591 for planning the future of child watching?

New changeset 3ccdd9b180f9a3f29c8ddc8ad1b331fe8df26519 by Benjamin Peterson in branch 'master':
closes bpo-38692: Add a pidfd child process watcher to asyncio. (GH-17069)
https://github.com/python/cpython/commit/3ccdd9b180f9a3f29c8ddc8ad1b331fe8df26519

Sorry for the late review.

PidfdChildWatcher should be enumerated in unix_events.py:__all__ to make the class visible by asyncio import rules.

Kyle, would you make a post-fix PR?

> PidfdChildWatcher should be enumerated in unix_events.py:__all__ to make the class visible by asyncio import rules.

> Kyle, would you make a post-fix PR?

I actually just noticed that myself and was coming back to the bpo issue to mention that it was missing from __all__. I'll take of that, no problem.

New changeset 3f8cebd32c1e6f20a1a1440e51c308a5f70ca959 by Miss Islington (bot) (Kyle Stanley) in branch 'master':
bpo-38692: Add asyncio.PidfdChildWatcher to __all__ (GH-17161)
https://github.com/python/cpython/commit/3f8cebd32c1e6f20a1a1440e51c308a5f70ca959

I have consistent behavior on Fedora 32 in mock [0] and podman [1]. Wanted to test docker as well, but my docker setup is currently broken.

# python3.9
Python 3.9.0a1 (default, Nov 20 2019, 00:00:00) 
[GCC 9.2.1 20190827 (Red Hat 9.2.1-1)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.pidfd_open(-1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
PermissionError: [Errno 1] Operation not permitted


=> the test fails.


[0] https://github.com/rpm-software-management/rpm
[1] https://podman.io/

BTW my kernel is: 5.3.7-301.fc31.x86_64

Sorry for commenting twice, I forgot to mention that.

> BTW my kernel is: 5.3.7-301.fc31.x86_64

I get a different error on Fedora 31 with Linux kernel 5.3.9-300.fc31.x86_64:

$ ./python
Python 3.9.0a1+ (heads/method_freelist:e34fa9b8d7, Nov 20 2019, 12:09:54) 
[GCC 9.2.1 20190827 (Red Hat 9.2.1-1)] on linux
>>> import os
>>> os.pidfd_open(-1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 22] Invalid argument

I don't know about podman, but it sounds like mock and docker both use buggy sandboxing: https://bugzilla.redhat.com/show_bug.cgi?id=1770154

I reopen the issue since the discussion restarted.

As I already proposed previously, I proposed PR 17290 to simply skip test_pidfd_open() if os.pidfd_open() fails with a PermissionError.

I don't propose to change os.pidfd_open(), only the test unit for practical reasons: not bother users who have to use a strict Linux sandbox using a syscalls whitelist.

New changeset 3ab479a2d1959923c9ab80c227dd1f39720b4e2d by Victor Stinner in branch 'master':
bpo-38692: Skip test_posix.test_pidfd_open() on EPERM (GH-17290)
https://github.com/python/cpython/commit/3ab479a2d1959923c9ab80c227dd1f39720b4e2d

I pushed my "Skip test_posix.test_pidfd_open() on EPERM" change just for practical reasons. We can hope that Linux sandboxes will shortly be updated to allow pidfd_open() syscall. It should be safe for most use cases ;-) I close again the issue.

Thank you very much for the commit "Skip test_posix.test_pidfd_open() on EPERM"! I can confirm it makes test_posix pass inside a systemd-nspawn container on Arch Linux. On the other hand, I found that tests were broken as both systemd libseccomp on Arch Linux were out-dated. With recently-pushed systemd 243.162-2 and locally-updated libseccomp 2.4.2, os.pidfd_open() works fine. Apparently [1] and [2] are relevant fixes.

By the way, upgrading systemd and libseccomp also fixes test_signal.PidfdSignalTest [3], which is added two days ago [4].

[1] https://github.com/systemd/systemd-stable/commit/51ea58a04b1851b31a14dfa7eec76254f5ddef16
[2] https://github.com/seccomp/libseccomp/commit/be65b26b67099be2b2b4890d736dbd1ad15adf36
[3] https://github.com/python/cpython/pull/17290#issuecomment-556869679
[4] https://bugs.python.org/issue38712

> I can confirm it makes test_posix pass inside a systemd-nspawn container on Arch Linux.

Great!