Use MultiLoopChildWatcher in tests where available#5862
Use MultiLoopChildWatcher in tests where available#5862webknjaz merged 2 commits intoaio-libs:masterfrom
MultiLoopChildWatcher in tests where available#5862Conversation
psf-chronographer
bot
added
the
bot:chronographer:provided
|
ok, will join the other thread. Wasn't sure where to look since there are many related tickets. |
|
Let's keep this open for now. I think we could accept it after some minor polishing. But I still need an answer to my question above, okay? I encourage you to participate in other discussions, though. And it's indeed better to merge small PRs. I only mentioned other efforts so that people interested in achieving the common goal could collaborate more effectively and not repeat each other's work twice. |
aiohttp/test_utils.py
Outdated
| if hasattr(asyncio, "MultiLoopChildWatcher"): | ||
| watcher = asyncio.MultiLoopChildWatcher() | ||
| else: | ||
| watcher = asyncio.SafeChildWatcher() |
There was a problem hiding this comment.
I have a few comments on this block: it is preferable to use the EAFP style in Python and also we need to have comments about the motivation of this.
To solve this, I suggest you cherry-pick my commit bea959a (#5431) (same patch: https://github.com/aio-libs/aiohttp/commit/bea959a4ce195405939c40f184c5189206064447.patch) and then change ThreadedChildWatcher with MultiLoopChildWatcher in a follow-up commit. If you know how to use interactive rebase in Git, it should be easy for you :)
Then, we could merge this change and backport it.
After that, I think it would be useful to vendor a backported watcher right in this repo and make use of it under older supported Pythons too. But this is a separate effort that should go into a separate PR.
There was a problem hiding this comment.
Thanks for the review.
I'm reading this ticket https://bugs.python.org/issue38591 and there seems to be many instabilities with MultiLoopChildWatcher. I think ThreadedChildWatcher might actually be a better choice here...
Regarding the performance concerns of creating a new thread, how about additionally exposing skip_watcher: bool as a param? SafeChildWatcher was originally introduced to support asyncio.create_subprocess_*. In my use case, this support is not needed. If we expose the param, no one pays the cost unnecessarily.
There was a problem hiding this comment.
Oh, maybe that's why I chose ThreadedChildWatcher. I think I talked to Andrew and he said to use it. It was still problematic under the heavy load on my laptop, IIRC.
As for skip_watcher, please file a proposal issue or a draft PR to demonstrate how you see it being exposed, API-wise.
sweatybridge
force-pushed
the
loop-watcher
branch
from
a022285 to
c6594ea
Compare
Codecov Report
@@ Coverage Diff @@
## master #5862 +/- ##
=======================================
Coverage 96.75% 96.75%
=======================================
Files 44 44
Lines 9848 9851 +3
Branches 1591 1591
=======================================
+ Hits 9528 9531 +3
Misses 182 182
Partials 138 138
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
webknjaz
changed the title
MultiLoopChildWatcher in tests where available
Backport to 3.8: 💚 backport PR created✅ Backport PR branch: Backported as #5863 🤖 @patchback |
PR #5862 Refs: * pytest-dev/pytest-xdist#620 * https://stackoverflow.com/a/58614689/595220 * https://bugs.python.org/issue35621 * python/cpython#14344 * https://docs.python.org/3/library/asyncio-policy.html#asyncio.MultiLoopChildWatcher Co-authored-by: Sviatoslav Sydorenko <[email protected]> (cherry picked from commit 7080a8b)
…s where available (#5863) Co-authored-by: Sviatoslav Sydorenko <[email protected]> Co-authored-by: Han Qiao <[email protected]>
2 participants
What do these changes do?
Python 3.8 adds a new child watcher class that doesn't require running loop in the main thread: https://docs.python.org/3/library/asyncio-policy.html#asyncio.MultiLoopChildWatcher. This PR uses the new watcher if it is available in asyncio module.
It should reduce test flakiness when parallelized with
pytest-xdist.Are there changes in behavior for the user?
No change for end users. Existing tests in
test_loop.pycovers this change given the current text matrix.Related issue number
#3450
#2075
Checklist
CONTRIBUTORS.txtCHANGESfolder<issue_id>.<type>for example (588.bugfix)issue_idchange it to the pr id after creating the pr.feature: Signifying a new feature..bugfix: Signifying a bug fix..doc: Signifying a documentation improvement..removal: Signifying a deprecation or removal of public API..misc: A ticket has been closed, but it is not of interest to users.