Fix timer handle churn in websocket heartbeat #8608

bdraco · 2024-08-05T12:27:56Z

What do these changes do?

Each message would cancel and reset the timer handler which meant asyncio would have to rebuild the whole schedule heap frequently because so many timer handlers were being created and than cancelled.

When asyncio timer handle cancels reach the threadhold the whole heap has to be rebuilt
https://github.com/python/cpython/blob/1422500d020bd199b26357fc387f8b79b82226cd/Lib/asyncio/base_events.py#L1968 which is extremely inefficent.

To solve this, when a timer handle is already running, instead of cancelling it, we let it fire and reschedule if heartbeat would be sent to early.

The websocket ping/pong code was missing some coverage in this area so there are some new tests as well.

Are there changes in behavior for the user?

no

Is it a substantial burden for the maintainers to support this?

no

Each message would cancel and reset the timer handler which meant asyncio would have to rebuild the whole schedule heap frequently because so many timer handlers were being created and than cancelled. When asyncio timer handle cancels reach the threadhold the whole heap has to be rebuilt https://github.com/python/cpython/blob/1422500d020bd199b26357fc387f8b79b82226cd/Lib/asyncio/base_events.py#L1968 which is extremely inefficent. To solve this, when a timer handle is already running, instead of cancelling it, we let it fire and reschedule if heartbeat would be sent to early.

aiohttp/client_ws.py

aiohttp/web_ws.py

aiohttp/client_ws.py

codecov · 2024-08-05T13:11:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.93%. Comparing base (b2691f2) to head (f9c6027).
Report is 854 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8608      +/-   ##
==========================================
+ Coverage   97.90%   97.93%   +0.03%     
==========================================
  Files         107      107              
  Lines       33515    33669     +154     
  Branches     3941     3952      +11     
==========================================
+ Hits        32813    32974     +161     
+ Misses        518      515       -3     
+ Partials      184      180       -4

Flag	Coverage Δ
CI-GHA	`97.85% <100.00%> (+0.03%)`	⬆️
OS-Linux	`97.51% <100.00%> (+0.03%)`	⬆️
OS-Windows	`95.87% <100.00%> (+0.01%)`	⬆️
OS-macOS	`97.17% <98.48%> (+0.02%)`	⬆️
Py-3.10.11	`97.32% <98.48%> (+0.02%)`	⬆️
Py-3.10.14	`97.26% <100.00%> (+0.03%)`	⬆️
Py-3.11.9	`97.49% <100.00%> (+0.03%)`	⬆️
Py-3.12.4	`97.62% <100.00%> (+0.03%)`	⬆️
Py-3.8.10	`95.64% <100.00%> (+0.01%)`	⬆️
Py-3.8.18	`97.16% <100.00%> (+0.03%)`	⬆️
Py-3.9.13	`97.31% <98.48%> (+0.02%)`	⬆️
Py-3.9.19	`97.25% <100.00%> (+0.03%)`	⬆️
Py-pypy7.3.16	`96.84% <100.00%> (+0.03%)`	⬆️
VM-macos	`97.17% <98.48%> (+0.02%)`	⬆️
VM-ubuntu	`97.51% <100.00%> (+0.03%)`	⬆️
VM-windows	`95.87% <100.00%> (+0.01%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bdraco · 2024-08-05T14:41:07Z

We don't have any tests for when the heartbeat would get rescheduled. Need to write some.

bdraco · 2024-08-05T22:09:29Z

95.2% reduction in websocket client timer handler churn. That's even with the downstream libs that already have their own ping/pong implementation.

I'm happy with this. I'll let it bake overnight and than write some tests tomorrow if I don't have any more fires from my day job to put out.

bdraco · 2024-08-05T22:46:26Z

95.1% reduction on the other instance I tested this on

CHANGES/8608.misc.rst

aiohttp/helpers.py

Dreamsorcerer

Looks good to me. I'm about to update these test files with type checking though, so would be great if you can update after those changes.

Co-authored-by: Sam Bull <[email protected]>

bdraco · 2024-08-06T16:38:58Z

Thanks!

tests/test_client_ws_functional.py

aiohttp/client_ws.py

patchback · 2024-08-07T21:58:59Z

Backport to 3.10: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply c4acabc on top of patchback/backports/3.10/c4acabc836ab969e95199aa976e85c01df720a27/pr-8608

Backporting merged PR #8608 into master

Ensure you have a local repo clone of your fork. Unless you cloned it
from the upstream, this would be your origin remote.
Make sure you have an upstream repo added as a remote too. In these
instructions you'll refer to it by the name upstream. If you don't
have it, here's how you can add it:
```
$ git remote add upstream https://github.com/aio-libs/aiohttp.git
```

Ensure you have the latest copy of upstream and prepare a branch
that will hold the backported code:

$ git fetch upstream
$ git checkout -b patchback/backports/3.10/c4acabc836ab969e95199aa976e85c01df720a27/pr-8608 upstream/3.10

Now, cherry-pick PR Fix timer handle churn in websocket heartbeat #8608 contents into that branch:
```
$ git cherry-pick -x c4acabc836ab969e95199aa976e85c01df720a27
```
If it'll yell at you with something like fatal: Commit c4acabc836ab969e95199aa976e85c01df720a27 is a merge but no -m option was given., add -m 1 as follows instead:
```
$ git cherry-pick -m1 -x c4acabc836ab969e95199aa976e85c01df720a27
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR Fix timer handle churn in websocket heartbeat #8608 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.10/c4acabc836ab969e95199aa976e85c01df720a27/pr-8608

Create a PR, ensure that the CI is green. If it's not — update it so that
the tests and any other checks pass. This is it!
Now relax and wait for the maintainers to process your pull request
when they have some cycles to do reviews. Don't worry — they'll tell you if
any improvements are necessary when the time comes!

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

patchback · 2024-08-07T21:59:03Z

Backport to 3.11: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply c4acabc on top of patchback/backports/3.11/c4acabc836ab969e95199aa976e85c01df720a27/pr-8608

Backporting merged PR #8608 into master

Ensure you have a local repo clone of your fork. Unless you cloned it
from the upstream, this would be your origin remote.
Make sure you have an upstream repo added as a remote too. In these
instructions you'll refer to it by the name upstream. If you don't
have it, here's how you can add it:
```
$ git remote add upstream https://github.com/aio-libs/aiohttp.git
```

Ensure you have the latest copy of upstream and prepare a branch
that will hold the backported code:

$ git fetch upstream
$ git checkout -b patchback/backports/3.11/c4acabc836ab969e95199aa976e85c01df720a27/pr-8608 upstream/3.11

Now, cherry-pick PR Fix timer handle churn in websocket heartbeat #8608 contents into that branch:
```
$ git cherry-pick -x c4acabc836ab969e95199aa976e85c01df720a27
```
If it'll yell at you with something like fatal: Commit c4acabc836ab969e95199aa976e85c01df720a27 is a merge but no -m option was given., add -m 1 as follows instead:
```
$ git cherry-pick -m1 -x c4acabc836ab969e95199aa976e85c01df720a27
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR Fix timer handle churn in websocket heartbeat #8608 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.11/c4acabc836ab969e95199aa976e85c01df720a27/pr-8608

Create a PR, ensure that the CI is green. If it's not — update it so that
the tests and any other checks pass. This is it!
Now relax and wait for the maintainers to process your pull request
when they have some cycles to do reviews. Don't worry — they'll tell you if
any improvements are necessary when the time comes!

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

Co-authored-by: Sam Bull <[email protected]> (cherry picked from commit c4acabc)

… heartbeat (#8639) Co-authored-by: Sam Bull <[email protected]> (cherry picked from commit c4acabc)

… heartbeat (#8640) Co-authored-by: Sam Bull <[email protected]> (cherry picked from commit c4acabc)

bdraco commented Aug 5, 2024

View reviewed changes

aiohttp/client_ws.py Outdated Show resolved Hide resolved

bdraco commented Aug 5, 2024

View reviewed changes

aiohttp/web_ws.py Outdated Show resolved Hide resolved

bdraco added 6 commits August 5, 2024 07:33

preen

ab09f18

preen

f59f732

fix merge

4ea43f9

fix merge

b1ed054

fix merge

807e334

fix merge

c3ca179

bdraco added backport-3.10 backport-3.11 Trigger automatic backporting to the 3.11 release branch by Patchback robot labels Aug 5, 2024

bdraco added 5 commits August 5, 2024 07:43

fix merge

8d0756b

fix merge

3321b93

fix merge

9a92850

fix merge

abc1d80

fix merge

5cb41f3

Dreamsorcerer reviewed Aug 5, 2024

View reviewed changes

aiohttp/client_ws.py Show resolved Hide resolved

bdraco added 2 commits August 5, 2024 19:19

add coverage for pong after many messages

179b0d5

changelog

fb71f93

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Aug 6, 2024

bdraco commented Aug 6, 2024

View reviewed changes

CHANGES/8608.misc.rst Outdated Show resolved Hide resolved

Update CHANGES/8608.misc.rst

0c84a8b

bdraco commented Aug 6, 2024

View reviewed changes

CHANGES/8608.misc.rst Outdated Show resolved Hide resolved

Update CHANGES/8608.misc.rst

02bd5c6

This was referenced Aug 6, 2024

Analyze additional places where there is timer handle churn #8613

Closed

Risk of websocket ping tasks being garbage collected before they finish #8614

Closed

bdraco reopened this Aug 6, 2024

bdraco marked this pull request as ready for review August 6, 2024 16:10

bdraco requested review from webknjaz and asvetlov as code owners August 6, 2024 16:10

Dreamsorcerer reviewed Aug 6, 2024

View reviewed changes

aiohttp/helpers.py Outdated Show resolved Hide resolved

Dreamsorcerer approved these changes Aug 6, 2024

View reviewed changes

Update aiohttp/helpers.py

6e7fa77

Co-authored-by: Sam Bull <[email protected]>

bdraco mentioned this pull request Aug 7, 2024

Reduce number of aiohttp.TCPConnector cleanup_closed checks to one per minute home-assistant/core#123268

Merged

19 tasks

bdraco added this to the 3.10.2 milestone Aug 7, 2024

bdraco added 4 commits August 7, 2024 10:59

Merge branch 'master' into timer_handle_churn

7a5f09e

lint

f533e86

fix type

5eec564

fix type

5e7230b

bdraco commented Aug 7, 2024

View reviewed changes

tests/test_client_ws_functional.py Outdated Show resolved Hide resolved

test the sending many in handler case as well

59df3db

bdraco commented Aug 7, 2024

View reviewed changes

aiohttp/client_ws.py Show resolved Hide resolved

bdraco added 2 commits August 7, 2024 16:08

Merge branch 'master' into timer_handle_churn

7807ce9

adjust test to make sure we reschedule once

f9c6027

bdraco merged commit c4acabc into master Aug 7, 2024
37 of 38 checks passed

bdraco deleted the timer_handle_churn branch August 7, 2024 21:58

bdraco added a commit that referenced this pull request Aug 7, 2024

Fix timer handle churn in websocket heartbeat (#8608)

b92861f

Co-authored-by: Sam Bull <[email protected]> (cherry picked from commit c4acabc)

bdraco added a commit that referenced this pull request Aug 7, 2024

Fix timer handle churn in websocket heartbeat (#8608)

6265731

Co-authored-by: Sam Bull <[email protected]> (cherry picked from commit c4acabc)

bdraco added a commit that referenced this pull request Aug 7, 2024

[PR #8608/c4acabc backport][3.10] Fix timer handle churn in websocket…

68e8496

… heartbeat (#8639) Co-authored-by: Sam Bull <[email protected]> (cherry picked from commit c4acabc)

bdraco added a commit that referenced this pull request Aug 7, 2024

[PR #8608/c4acabc backport][3.11] Fix timer handle churn in websocket…

13ef3c1

… heartbeat (#8640) Co-authored-by: Sam Bull <[email protected]> (cherry picked from commit c4acabc)

bdraco mentioned this pull request Aug 8, 2024

Revert "Significantly reduce heartbeat overhead (#301)" home-assistant-libs/aioshelly#611

Merged

Dreamsorcerer mentioned this pull request Sep 10, 2024

gh-122881: Reduce asyncio heapq scheduling overhead python/cpython#122882

Closed

bdraco mentioned this pull request Nov 1, 2024

Avoid starting connection timeout when a connection is already available #9600

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix timer handle churn in websocket heartbeat #8608

Fix timer handle churn in websocket heartbeat #8608

bdraco commented Aug 5, 2024 •

edited

Loading

codecov bot commented Aug 5, 2024 •

edited

Loading

bdraco commented Aug 5, 2024

bdraco commented Aug 5, 2024

bdraco commented Aug 5, 2024

Dreamsorcerer left a comment

bdraco commented Aug 6, 2024

patchback bot commented Aug 7, 2024 •

edited

Loading

patchback bot commented Aug 7, 2024 •

edited

Loading

Fix timer handle churn in websocket heartbeat #8608

Fix timer handle churn in websocket heartbeat #8608

Conversation

bdraco commented Aug 5, 2024 • edited Loading

What do these changes do?

Are there changes in behavior for the user?

Is it a substantial burden for the maintainers to support this?

codecov bot commented Aug 5, 2024 • edited Loading

Codecov Report

bdraco commented Aug 5, 2024

bdraco commented Aug 5, 2024

bdraco commented Aug 5, 2024

Dreamsorcerer left a comment

Choose a reason for hiding this comment

bdraco commented Aug 6, 2024

patchback bot commented Aug 7, 2024 • edited Loading

Backport to 3.10: 💔 cherry-picking failed — conflicts found

Backporting merged PR #8608 into master

patchback bot commented Aug 7, 2024 • edited Loading

Backport to 3.11: 💔 cherry-picking failed — conflicts found

Backporting merged PR #8608 into master

bdraco commented Aug 5, 2024 •

edited

Loading

codecov bot commented Aug 5, 2024 •

edited

Loading

patchback bot commented Aug 7, 2024 •

edited

Loading

patchback bot commented Aug 7, 2024 •

edited

Loading