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.

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

Switch to ThreadedChildWatcher and test #5877

Merged
merged 9 commits into from
Jul 28, 2021
Merged

Switch to ThreadedChildWatcher and test #5877

Show file tree
Hide file tree
Changes from 5 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 `ThreadedChildWatcher` under POSIX to allow setting up test loop in non-main thread.
sweatybridge marked this conversation as resolved.
Show resolved Hide resolved
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
13 changes: 12 additions & 1 deletion tests/test_loop.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import pytest

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


@pytest.mark.skipif(
Expand Down Expand Up @@ -43,3 +43,14 @@ def test_default_loop(self) -> None:

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


def test_setup_loop_non_main_thread() -> None:
def target() -> None:
with loop_context():
pass
Copy link
Member

Choose a reason for hiding this comment

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

Could we run something in this loop and maybe process some signal? This is what may go wrong potentially.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting up the loop context in python 3.7 still uses SafeChildWatcher, which in turn registers signals. I've updated the test to expect failure on python 3.7 and success in 3.8+. This way we have both code paths covered.

Subsequently after merging pytest-xdist, we might see occasional failures on python 3.7 tests in CI for the same reason. This could happen with any tests that use the asyncio event loop, not specific to this test alone. This will be ok once we deprecate 3.7.

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather vendor a backport and use it under Python 3.7. Also, it's not yet time to drop it anyway. aiohttp 3.8 still supports Python 3.6 even. It'll be EOL in 5 months but for CPython 3.7 there are 2 years to go.
Being a framework/library forces us to support a wider range of versions, unlike projects that are just apps bound to a single env.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, agree that backporting is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here's the backport with manually resolved merge conflicts #5919


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