-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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
Changes from 4 commits
e49365f
49cfa81
3a3d5e2
60dafa1
16f1c4e
c8ae89d
8229efc
6473022
02190f2
9607d8e
dad94d9
6e56155
1037078
4e69faf
740e0c2
7721ea1
f4f6fc5
cad5231
5647edb
102ba29
de21df0
d30824f
978c822
fbeb1a7
6fe4985
592fcb9
d27f174
871507f
5dfc542
a4fac11
fc2e400
65a2db6
d25f852
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -712,12 +712,12 @@ def setUp(self): | |
self.set_event_loop(self.loop) | ||
|
||
|
||
class GenericWatcherTests: | ||
class GenericWatcherTests(test_utils.TestCase): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this test wasn't enabled? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this was patching the wrong attribute - asyncio calls |
||
asyncio.set_child_watcher(watcher) | ||
graingert marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
@@ -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
|
||
|
There was a problem hiding this comment.
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.)