[issue38692] add a pidfd child process watcher

Change by Eryk Sun : -- Removed message: https://bugs.python.org/msg405695 ___ Python tracker ___ ___ Python-bugs-list mailing list

[issue38692] add a pidfd child process watcher

Change by Eryk Sun : -- components: +asyncio -Build nosy: +asvetlov, yselivanov -ahmedsayeed1982 versions: +Python 3.9 -Python 3.11 ___ Python tracker ___ _

[issue38692] add a pidfd child process watcher

Ahmed Sayeed added the comment: --8<--- 1 size_t fwrite(const void * __restrict ptr, size_t size, http://www-look-4.com/category/travel/ 2 size_t nmemb, register FILE * __restrict stream) 3 { 4 size_t retval; https://komiya-dental.com/category/technology/ 5 _

[issue38692] add a pidfd child process watcher

Change by STINNER Victor : -- nosy: -vstinner ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.p

[issue38692] add a pidfd child process watcher

Change by Chih-Hsuan Yen : -- nosy: -yan12125 ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.p

[issue38692] add a pidfd child process watcher

Change by Saiyang Gou : -- nosy: +gousaiyang nosy_count: 10.0 -> 11.0 pull_requests: +24233 pull_request: https://github.com/python/cpython/pull/25515 ___ Python tracker ___ __

[issue38692] add a pidfd child process watcher

STINNER Victor added the comment: > I can confirm it makes test_posix pass inside a systemd-nspawn container on > Arch Linux. Great! -- ___ Python tracker ___ __

[issue38692] add a pidfd child process watcher

Chih-Hsuan Yen added the comment: 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 Linu

[issue38692] add a pidfd child process watcher

STINNER Victor added the comment: 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. --

[issue38692] add a pidfd child process watcher

STINNER Victor added the comment: 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 -- _

[issue38692] add a pidfd child process watcher

STINNER Victor added the comment: 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 st

[issue38692] add a pidfd child process watcher

Change by STINNER Victor : -- pull_requests: +16783 stage: resolved -> patch review pull_request: https://github.com/python/cpython/pull/17290 ___ Python tracker ___ __

[issue38692] add a pidfd child process watcher

STINNER Victor added the comment: I reopen the issue since the discussion restarted. -- status: closed -> open ___ Python tracker ___ _

[issue38692] add a pidfd child process watcher

STINNER Victor added the comment: > 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 >>> i

[issue38692] add a pidfd child process watcher

Nathaniel Smith added the comment: 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 -- ___ Python tracker

[issue38692] add a pidfd child process watcher

Miro Hrončok added the comment: BTW my kernel is: 5.3.7-301.fc31.x86_64 Sorry for commenting twice, I forgot to mention that. -- ___ Python tracker ___ ___

[issue38692] add a pidfd child process watcher

Miro Hrončok added the comment: 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",

[issue38692] add a pidfd child process watcher

Change by Kyle Stanley : -- stage: patch review -> resolved status: open -> closed ___ Python tracker ___ ___ Python-bugs-list maili

[issue38692] add a pidfd child process watcher

miss-islington added the comment: 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

[issue38692] add a pidfd child process watcher

Change by Kyle Stanley : -- pull_requests: +16671 stage: resolved -> patch review pull_request: https://github.com/python/cpython/pull/17161 ___ Python tracker ___

[issue38692] add a pidfd child process watcher

Kyle Stanley added the comment: > 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 missi

[issue38692] add a pidfd child process watcher

Andrew Svetlov added the comment: 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? -- resolution: fixed -> status: closed -> open _

[issue38692] add a pidfd child process watcher

Benjamin Peterson added the comment: 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 -

[issue38692] add a pidfd child process watcher

Benjamin Peterson added the comment: 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? -- ___ Python tracker ___

[issue38692] add a pidfd child process watcher

Kyle Stanley added the comment: > > 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 th

[issue38692] add a pidfd child process watcher

Kyle Stanley added the comment: > 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 th

[issue38692] add a pidfd child process watcher

Yury Selivanov added the comment: 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

[issue38692] add a pidfd child process watcher

Benjamin Peterson added the comment: It will be fixed, though, as soon as the user upgrades systemd. -- ___ Python tracker ___ ___

[issue38692] add a pidfd child process watcher

STINNER Victor added the comment: > 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 th

[issue38692] add a pidfd child process watcher

Benjamin Peterson added the comment: 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

[issue38692] add a pidfd child process watcher

STINNER Victor added the comment: > 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 conf

[issue38692] add a pidfd child process watcher

Benjamin Peterson added the comment: 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. -- ___ Python tracker

[issue38692] add a pidfd child process watcher

STINNER Victor added the comment: > 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 EPE

[issue38692] add a pidfd child process watcher

Chih-Hsuan Yen added the comment: 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

[issue38692] add a pidfd child process watcher

Benjamin Peterson added the comment: It seems like systemd-nspawn is just breaking everything: https://sourceware.org/ml/libc-alpha/2019-11/msg00277.html -- ___ Python tracker __

[issue38692] add a pidfd child process watcher

Kyle Stanley added the comment: [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 -- ___

[issue38692] add a pidfd child process watcher

Kyle Stanley added the comment: > [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

[issue38692] add a pidfd child process watcher

Kyle Stanley added the comment: > 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 Benjam

[issue38692] add a pidfd child process watcher

Benjamin Peterson added the comment: 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. -- ___ Python tracker ___

[issue38692] add a pidfd child process watcher

Chih-Hsuan Yen added the comment: I got a failure in newly added test_pidfd_open: == FAIL: test_pidfd_open (test.test_posix.PosixTester) -- Traceback (most re

[issue38692] add a pidfd child process watcher

Benjamin Peterson added the comment: On Wed, Nov 6, 2019, at 09:57, STINNER Victor wrote: > > STINNER Victor 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

[issue38692] add a pidfd child process watcher

STINNER Victor added the comment: Would it be useful to use a pidfd in subprocess.Popen to fix bpo-38630 root issue, when pidfd is available? -- ___ Python tracker ___ __

[issue38692] add a pidfd child process watcher

Change by Benjamin Peterson : -- pull_requests: +16578 pull_request: https://github.com/python/cpython/pull/17069 ___ Python tracker ___ ___

[issue38692] add a pidfd child process watcher

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

[issue38692] add a pidfd child process watcher

Benjamin Peterson added the comment: pidfd_open was added after pidfd pollling, so it should suffice to make sure pidfd_open doesn't ENOSYS. -- ___ Python tracker ___ ___

[issue38692] add a pidfd child process watcher

Nathaniel Smith added the comment: 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 hav

[issue38692] add a pidfd child process watcher

Kyle Stanley added the comment: > 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):

[issue38692] add a pidfd child process watcher

STINNER Victor added the comment: See also bpo-38630 "subprocess.Popen.send_signal() should poll the process". -- ___ Python tracker ___ __

[issue38692] add a pidfd child process watcher

Benjamin Peterson added the comment: 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. -- ___ Python tracker

[issue38692] add a pidfd child process watcher

Nathaniel Smith added the comment: 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

[issue38692] add a pidfd child process watcher

Change by Benjamin Peterson : -- pull_requests: +16571 stage: -> patch review pull_request: https://github.com/python/cpython/pull/17063 ___ Python tracker ___ ___

[issue38692] add a pidfd child process watcher

STINNER Victor added the comment: +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. -- __

[issue38692] add a pidfd child process watcher

Change by STINNER Victor : -- nosy: +nanjekyejoannah ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://

[issue38692] add a pidfd child process watcher

STINNER Victor added the comment: 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

[issue38692] add a pidfd child process watcher

Andrew Svetlov added the comment: Nathaniel, you may be interested in the pidfd_open(), at least in adding the function to os module. -- nosy: +njs ___ Python tracker ___ ___

[issue38692] add a pidfd child process watcher

Andrew Svetlov added the comment: 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 fallba

[issue38692] add a pidfd child process watcher

New submission from Benjamin Peterson : 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 test