Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-94182: run the PidfdChildWatcher on the running loop #94184

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
e49365f
run the PidfdChildWatcher on the running loop
graingert Jun 23, 2022
49cfa81
enable GenericWatcherTests and add a test for pidfd watcher run in a …
graingert Jun 23, 2022
3a3d5e2
fix authspec typo
graingert Jun 23, 2022
60dafa1
Update Lib/test/test_asyncio/test_subprocess.py
graingert Jun 23, 2022
16f1c4e
Update Lib/test/test_asyncio/test_subprocess.py
graingert Jun 23, 2022
c8ae89d
Update Lib/test/test_asyncio/test_subprocess.py
graingert Jun 23, 2022
8229efc
Update Lib/test/test_asyncio/test_subprocess.py
graingert Jun 23, 2022
6473022
Merge branch 'main' into run-pidfdchildwatcher-on-the-running-loop
graingert Jun 23, 2022
02190f2
AbstractChildWatcher is called as a context manager
graingert Jun 24, 2022
9607d8e
move GenericWatcherTests into the other unix tests
graingert Jun 24, 2022
dad94d9
Merge branch 'main' into run-pidfdchildwatcher-on-the-running-loop
graingert Jun 24, 2022
6e56155
check that asyncio.set_event_loop() was not called
graingert Jun 24, 2022
1037078
📜🤖 Added by blurb_it.
blurb-it[bot] Jun 24, 2022
4e69faf
Merge branch 'main' into run-pidfdchildwatcher-on-the-running-loop
graingert Jun 24, 2022
740e0c2
Merge branch 'main' into run-pidfdchildwatcher-on-the-running-loop
graingert Jun 28, 2022
7721ea1
Merge branch 'main' into run-pidfdchildwatcher-on-the-running-loop
graingert Jul 6, 2022
f4f6fc5
Merge branch 'main' of github.com:python/cpython into run-pidfdchildw…
graingert Jul 7, 2022
cad5231
Merge branch 'main' into run-pidfdchildwatcher-on-the-running-loop
graingert Jul 19, 2022
5647edb
asyncio.run sets up the policy system asyncio.Runner with a loop_fact…
graingert Jul 19, 2022
102ba29
Update Misc/NEWS.d/next/Library/2022-06-24-08-49-47.gh-issue-94182.Wk…
graingert Jul 19, 2022
de21df0
Update Misc/NEWS.d/next/Library/2022-06-24-08-49-47.gh-issue-94182.Wk…
graingert Jul 20, 2022
d30824f
move duplicate has_pidfd_support to module global
graingert Jul 20, 2022
978c822
Update Lib/test/test_asyncio/test_subprocess.py
graingert Jul 20, 2022
fbeb1a7
Merge branch 'main' into run-pidfdchildwatcher-on-the-running-loop
graingert Jul 21, 2022
6fe4985
Update Lib/test/test_asyncio/test_subprocess.py
graingert Jul 21, 2022
592fcb9
Merge branch 'main' into run-pidfdchildwatcher-on-the-running-loop
graingert Aug 2, 2022
d27f174
Merge branch 'main' into run-pidfdchildwatcher-on-the-running-loop
graingert Aug 11, 2022
871507f
Merge branch 'main' into run-pidfdchildwatcher-on-the-running-loop
graingert Aug 12, 2022
5dfc542
Merge branch 'main' into run-pidfdchildwatcher-on-the-running-loop
gvanrossum Oct 7, 2022
a4fac11
Merge branch 'main' into run-pidfdchildwatcher-on-the-running-loop
gvanrossum Oct 7, 2022
fc2e400
Update Misc/NEWS.d/next/Library/2022-06-24-08-49-47.gh-issue-94182.Wk…
1st1 Oct 7, 2022
65a2db6
Update Misc/NEWS.d/next/Library/2022-06-24-08-49-47.gh-issue-94182.Wk…
1st1 Oct 7, 2022
d25f852
Merge branch 'main' into run-pidfdchildwatcher-on-the-running-loop
JelleZijlstra Oct 7, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 12 additions & 32 deletions Lib/asyncio/unix_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -911,46 +911,29 @@ class PidfdChildWatcher(AbstractChildWatcher):
recent (5.3+) kernels.
"""

def __init__(self):
self._loop = None
self._callbacks = {}

def __enter__(self):
return self

def __exit__(self, exc_type, exc_value, exc_traceback):
pass

def is_active(self):
return self._loop is not None and self._loop.is_running()
return True

def close(self):
self.attach_loop(None)
pass

def attach_loop(self, loop):
if self._loop is not None and loop is None and self._callbacks:
warnings.warn(
'A loop is being detached '
'from a child watcher with pending handlers',
RuntimeWarning)
for pidfd, _, _ in self._callbacks.values():
self._loop._remove_reader(pidfd)
os.close(pidfd)
self._callbacks.clear()
self._loop = loop
pass

def add_child_handler(self, pid, callback, *args):
existing = self._callbacks.get(pid)
if existing is not None:
self._callbacks[pid] = existing[0], callback, args
else:
pidfd = os.pidfd_open(pid)
self._loop._add_reader(pidfd, self._do_wait, pid)
self._callbacks[pid] = pidfd, callback, args
loop = events.get_running_loop()
pidfd = os.pidfd_open(pid)
loop._add_reader(pidfd, self._do_wait, pid, pidfd, callback, args)

def _do_wait(self, pid):
pidfd, callback, args = self._callbacks.pop(pid)
self._loop._remove_reader(pidfd)
def _do_wait(self, pid, pidfd, callback, args):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why we can't pass the loop as an argument as well? (Though maybe that creates an unhealthy self-reference to the loop.)

loop = events.get_running_loop()
loop._remove_reader(pidfd)
try:
_, status = os.waitpid(pid, 0)
except ChildProcessError:
Expand All @@ -968,12 +951,9 @@ def _do_wait(self, pid):
callback(pid, returncode, *args)

def remove_child_handler(self, pid):
try:
pidfd, _, _ = self._callbacks.pop(pid)
except KeyError:
return False
self._loop._remove_reader(pidfd)
os.close(pidfd)
# asyncio never calls remove_child_handler() !!!
# The method is no-op but is implemented because
# abstract base classes require it.
return True


Expand Down
39 changes: 36 additions & 3 deletions Lib/test/test_asyncio/test_subprocess.py
Original file line number Diff line number Diff line change
Expand Up @@ -712,12 +712,12 @@ def setUp(self):
self.set_event_loop(self.loop)


class GenericWatcherTests:
class GenericWatcherTests(test_utils.TestCase):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test wasn't enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split this fix out, so this PR is easier to review: #95009


def test_create_subprocess_fails_with_inactive_watcher(self):

async def execute():
watcher = mock.create_authspec(asyncio.AbstractChildWatcher)
watcher = mock.create_autopec(asyncio.AbstractChildWatcher)
graingert marked this conversation as resolved.
Show resolved Hide resolved
watcher.is_active.return_value = False
Copy link
Contributor Author

@graingert graingert Jun 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was patching the wrong attribute - asyncio calls asyncio.get_child_watcher().__enter__().is_active()

asyncio.set_child_watcher(watcher)
graingert marked this conversation as resolved.
Show resolved Hide resolved

Expand All @@ -727,9 +727,42 @@ async def execute():

watcher.add_child_handler.assert_not_called()

self.assertIsNone(self.loop.run_until_complete(execute()))
self.assertIsNone(asyncio.run(execute()))


def has_pidfd_support():
if not hasattr(os, 'pidfd_open'):
return False
try:
os.close(os.pidfd_open(os.getpid()))
except OSError:
return False
return True

@unittest.skipUnless(
has_pidfd_support(),
"operating system does not support pidfds",
)
def test_create_subprocess_with_pidfd(self):
async def in_thread():
proc = await asyncio.create_subprocess_exec(
*args,
graingert marked this conversation as resolved.
Show resolved Hide resolved
stdin=subprocess.PIPE,
stdout=subprocess.PIPE,
)
stdout, stderr = await proc.communicate(b"some data")
return proc.returncode, stdout

async def main():
return await asyncio.to_thread(asyncio.run, in_thread())

asyncio.set_child_watcher(asyncio.PidfdChildWatcher())
try:
returncode, stdout = asyncio.run(main())
self.assertEqual(returncode, 0)
self.assertEqual(stdout, b'some data')
finally:
asyncio.set_child_watcher(None)


if __name__ == '__main__':
graingert marked this conversation as resolved.
Show resolved Hide resolved
Expand Down