Use `MultiLoopChildWatcher` in tests where available #5862

sweatybridge · 2021-07-07T11:19:56Z

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.py covers this change given the current text matrix.

Related issue number

#3450
#2075

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the CHANGES folder
- name it <issue_id>.<type> for example (588.bugfix)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .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.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

webknjaz · 2021-07-07T11:23:53Z

This is already being introduced in #5431 but with a threaded watcher. It's also being discussed in #5852. Why do you think that multiloop would be a better fit? Do you want to join those discussions instead?

sweatybridge · 2021-07-07T11:26:13Z

ok, will join the other thread. Wasn't sure where to look since there are many related tickets.

webknjaz · 2021-07-07T11:30:35Z

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.

CHANGES/3450.bugfix

webknjaz · 2021-07-07T11:42:53Z

aiohttp/test_utils.py

+        if hasattr(asyncio, "MultiLoopChildWatcher"):
+            watcher = asyncio.MultiLoopChildWatcher()
+        else:
+            watcher = asyncio.SafeChildWatcher()


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.

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.

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.

Refs: * pytest-dev/pytest-xdist#620 * https://stackoverflow.com/a/58614689/595220 * https://bugs.python.org/issue35621 * python/cpython#14344

codecov · 2021-07-07T12:40:08Z

Codecov Report

Merging #5862 (c6594ea) into master (5b23426) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           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

Flag	Coverage Δ
unit	`96.65% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiohttp/test_utils.py	`99.68% <100.00%> (+<0.01%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b23426...c6594ea. Read the comment docs.

patchback · 2021-07-07T12:42:35Z

Backport to 3.8: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.8/7080a8bba86db1ea38372183a90d82bfa7d6d8b1/pr-5862

Backported as #5863

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

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]>

sweatybridge requested review from asvetlov and webknjaz as code owners July 7, 2021 11:19

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jul 7, 2021

sweatybridge closed this Jul 7, 2021

webknjaz reopened this Jul 7, 2021

webknjaz reviewed Jul 7, 2021

View reviewed changes

CHANGES/3450.bugfix Outdated Show resolved Hide resolved

webknjaz reviewed Jul 7, 2021

View reviewed changes

webknjaz and others added 2 commits July 7, 2021 19:52

Use ThreadedChildWatcher in tests where available

a0c69ca

Refs: * pytest-dev/pytest-xdist#620 * https://stackoverflow.com/a/58614689/595220 * https://bugs.python.org/issue35621 * python/cpython#14344

Use multi loop child watcher when possible

c6594ea

sweatybridge force-pushed the loop-watcher branch from a022285 to c6594ea Compare July 7, 2021 11:53

webknjaz added the backport-3.8 label Jul 7, 2021

webknjaz enabled auto-merge (squash) July 7, 2021 12:35

webknjaz disabled auto-merge July 7, 2021 12:35

webknjaz changed the title ~~Use MultiLoopChildWatcher when setting up test loop~~ Use MultiLoopChildWatcher in tests where available Jul 7, 2021

webknjaz enabled auto-merge (squash) July 7, 2021 12:41

webknjaz merged commit 7080a8b into aio-libs:master Jul 7, 2021

patchback bot mentioned this pull request Jul 7, 2021

[PR #5862/7080a8bb backport][3.8] Use MultiLoopChildWatcher in tests where available #5863

Merged

5 tasks

webknjaz mentioned this pull request Jul 7, 2021

Ascola/add parallel testing #5852

Closed

5 tasks

webknjaz added a commit that referenced this pull request Jul 7, 2021

[PR #5862/7080a8bb backport][3.8] Use MultiLoopChildWatcher in test…

68f0262

…s where available (#5863) Co-authored-by: Sviatoslav Sydorenko <[email protected]> Co-authored-by: Han Qiao <[email protected]>

sweatybridge mentioned this pull request Jul 14, 2021

Switch to ThreadedChildWatcher and test #5877

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `MultiLoopChildWatcher` in tests where available #5862

Use `MultiLoopChildWatcher` in tests where available #5862

sweatybridge commented Jul 7, 2021

webknjaz commented Jul 7, 2021

sweatybridge commented Jul 7, 2021

webknjaz commented Jul 7, 2021

webknjaz Jul 7, 2021

sweatybridge Jul 7, 2021

webknjaz Jul 7, 2021

codecov bot commented Jul 7, 2021 •

edited

Loading

patchback bot commented Jul 7, 2021 •

edited

Loading

Use MultiLoopChildWatcher in tests where available #5862

Use MultiLoopChildWatcher in tests where available #5862

Conversation

sweatybridge commented Jul 7, 2021

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

webknjaz commented Jul 7, 2021

sweatybridge commented Jul 7, 2021

webknjaz commented Jul 7, 2021

webknjaz Jul 7, 2021

Choose a reason for hiding this comment

sweatybridge Jul 7, 2021

Choose a reason for hiding this comment

webknjaz Jul 7, 2021

Choose a reason for hiding this comment

codecov bot commented Jul 7, 2021 • edited Loading

Codecov Report

patchback bot commented Jul 7, 2021 • edited Loading

Backport to 3.8: 💚 backport PR created

Use `MultiLoopChildWatcher` in tests where available #5862

Use `MultiLoopChildWatcher` in tests where available #5862

codecov bot commented Jul 7, 2021 •

edited

Loading

patchback bot commented Jul 7, 2021 •

edited

Loading