Skip to content

Switch to ThreadedChildWatcher and test #5877

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Jul 28, 2021
Merged

Switch to ThreadedChildWatcher and test #5877

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
dc361c4
Switch to ThreadedChildWatcher and test
Jul 14, 2021
95b0e5d
Add to changes
Jul 14, 2021
6b2d5af
Fix mypy
Jul 14, 2021
3072ae0
Tear down test loop in thread
Jul 14, 2021
8af64ab
Fix mypy
Jul 14, 2021
3b51df0
Fix changelog
Jul 27, 2021
0d624ca
Create subprocess in test loop
Jul 27, 2021
b56b139
Fail test on python 3.7
Jul 27, 2021
b99f35c
Update tests/test_loop.py
sweatybridge Jul 28, 2021
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
1 change: 1 addition & 0 deletions CHANGES/5877.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Uses :py:class:`~asyncio.ThreadedChildWatcher` under POSIX to allow setting up test loop in non-main thread.
2 changes: 1 addition & 1 deletion aiohttp/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,7 +497,7 @@ def setup_test_loop(
# * https://stackoverflow.com/a/58614689/595220
# * https://bugs.python.org/issue35621
# * https://github.com/python/cpython/pull/14344
watcher = asyncio.MultiLoopChildWatcher()
watcher = asyncio.ThreadedChildWatcher()
except AttributeError: # Python < 3.8
watcher = asyncio.SafeChildWatcher()
watcher.attach_loop(loop)
Expand Down
26 changes: 24 additions & 2 deletions tests/test_loop.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,15 @@
import pytest

from aiohttp import web
from aiohttp.test_utils import AioHTTPTestCase
from aiohttp.helpers import PY_38
from aiohttp.test_utils import AioHTTPTestCase, loop_context


@pytest.mark.skipif(
platform.system() == "Windows", reason="the test is not valid for Windows"
)
async def test_subprocess_co(loop: Any) -> None:
assert threading.current_thread() is threading.main_thread()
assert PY_38 or threading.current_thread() is threading.main_thread()
proc = await asyncio.create_subprocess_shell(
"exit 0",
stdin=asyncio.subprocess.DEVNULL,
Expand Down Expand Up @@ -43,3 +44,24 @@ def test_default_loop(self) -> None:

def test_default_loop(loop: Any) -> None:
assert asyncio.get_event_loop() is loop


@pytest.mark.xfail(not PY_38, reason="ThreadedChildWatcher is only available in 3.8+")
def test_setup_loop_non_main_thread() -> None:
child_exc = None

def target() -> None:
try:
with loop_context() as loop:
assert asyncio.get_event_loop() is loop
loop.run_until_complete(test_subprocess_co(loop))
Copy link
Member

Choose a reason for hiding this comment

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

I see this calls subprocesses but what about registering signal handlers? This is what actually fails on old watchers.

Copy link
Contributor Author

@sweatybridge sweatybridge Jul 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Registering signal handlers will always fail in non-main thread, regardless of whether we are using old or new watcher. The new watcher avoids this issue by not registering signal handlers at all (using waitpid as alternative).

What we want to ensure in this test is that child processes can be watched, not whether signals can be successfully registered. In other words, since the library no longer registers signal handler on python 3.8, it's not relevant to cover signal registering in tests.

Am I misunderstanding something?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. The original problem with xdist was exactly the fact that there's a problem with the signal handlers that happens in an inconsistent manner. This is what blocks #5431 and all the previous attempts. I was sure that the new watchers were supposed to fix this based on what @asvetlov mentioned to me privately. Are you sure it's still problematic?

Copy link
Contributor Author

@sweatybridge sweatybridge Jul 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

What I'm saying is signal handling is an independent problem from watching child process. The old watcher uses signal handling and will crash when mixed with xdist. The new watcher doesn't use signal handling and therefore is not problematic. Our test only need to verify that child process is watched, it doesn't matter whether it's implemented signals or not.

For reference, here's cpython's implementation for ThreadedChildWatcher https://github.com/python/cpython/blob/3.8/Lib/asyncio/unix_events.py#L1296. It starts a new thread and calls waitpid.

Compare that with SafeChildWatcher which inherits from BaseChildWatcher https://github.com/python/cpython/blob/3.8/Lib/asyncio/unix_events.py#L927. It tries to register signal handler and hence will raise an exception in non-main thread.

except Exception as exc:
nonlocal child_exc
child_exc = exc

# Ensures setup_test_loop can be called by pytest-xdist in non-main thread.
t = threading.Thread(target=target)
t.start()
t.join()

assert child_exc is None