Fix WebSocket reader flow control calculations

[bdraco]

Fix WebSocket reader flow control calculations

It appears this does not work correctly on master, as the calculated the size of every WebSocket message is 3 which is the length of the WSMessage tuple, and since the limit was enforced at 2x, there would have to be 43690 queued WebSocket messages before reading would pause.

This works well enough on 3.11 and 3.10 because we send the size in

aiohttp/aiohttp/streams.py

Line 700 in 49f65e6

def feed_data(self, data: _T, size: int = 0) -> None:

For master, I've moved the size into the WSMessage

originally posted by Dreamsorcerer in #9659 (comment) :

As DataQueue doesn't have _size anymore, the Generic should not require Sized anymore. Also, probably want to have WSMessage queues move to that, as they don't need the _size calculations.

The queues have been split into DataQueue and WebSocketDataQueue. Sized has been removed from DataQueue. WebSocketDataQueue is own class now because I didn't want to cythonize DataQueue, and it doesn't use many of the methods on the former DataQueue base class anyways.

This wasn't broken in any public stable release so no bugfix changelog message.

It likely wouldn't warrant a changelog message if it wasn't for FlowControlDataQueue was available for import in aiohttp top level namespace, although it would be a bit unexpected that someone would use it externally, its technically a breaking change.

[codspeed-hq]

CodSpeed Performance Report

Merging #9685 will not alter performance

_{Comparing websocket_flow_control (6c174e2) with master (e85db24)}

Summary

✅ 14 untouched benchmarks

[bdraco]

This feels like it's at the wrong level. We shouldn't be counting the data once it's parsed. We should be looking at it before it's parsed

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.67%. Comparing base (e85db24) to head (6c174e2).
Report is 28 commits behind head on master.

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #9685   +/-   ##
=======================================
  Coverage   98.67%   98.67%           
=======================================
  Files         116      117    +1     
  Lines       35732    35791   +59     
  Branches     4237     4241    +4     
=======================================
+ Hits        35258    35317   +59     
  Misses        319      319           
  Partials      155      155           

Flag	Coverage Δ
CI-GHA	98.56% <100.00%> (+<0.01%)	⬆️
OS-Linux	98.23% <100.00%> (+<0.01%)	⬆️
OS-Windows	95.99% <66.47%> (-0.14%)	⬇️
OS-macOS	97.30% <68.30%> (-0.14%)	⬇️
Py-3.10.11	97.16% <68.30%> (-0.13%)	⬇️
Py-3.10.15	97.72% <100.00%> (+<0.01%)	⬆️
Py-3.11.10	97.77% <100.00%> (+<0.01%)	⬆️
Py-3.11.9	97.23% <68.30%> (-0.13%)	⬇️
Py-3.12.7	98.26% <100.00%> (+<0.01%)	⬆️
Py-3.13.0	98.25% <100.00%> (+<0.01%)	⬆️
Py-3.9.13	97.08% <68.30%> (-0.13%)	⬇️
Py-3.9.20	97.63% <100.00%> (+<0.01%)	⬆️
Py-pypy7.3.16	97.25% <99.45%> (+<0.01%)	⬆️
VM-macos	97.30% <68.30%> (-0.14%)	⬇️
VM-ubuntu	98.23% <100.00%> (+<0.01%)	⬆️
VM-windows	95.99% <66.47%> (-0.14%)	⬇️

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]

This still feels wrong because we miss a large percentage of the actual payload since the header is decoded before its counted

[Dreamsorcerer]

This still feels wrong because we miss a large percentage of the actual payload since the header is decoded before its counted

I feel like the headers are a separate thing. This is only concerned with the amount of websocket data queued up. What it is missing is the msg.extra, which could technically be of any length I believe (but, mostly only used on EXIT messages, if I remember correctly, so probably not an issue with well-behaving servers).

[Dreamsorcerer]

I have no opinion on which of these 2 PRs we go with. I'm fine with both, though I can imagine some users may want to restrict memory usage.

[bdraco]

I'm pretty happy with this now. It does need some new tests to ensure the flow control calculation remains meaningful

[bdraco]

Maybe it would be best to do the tests as xfail, backport them to 3.10/3.11, and remove the xfail there so we get coverage down the chain

[bdraco]

xfail tests unmarked xfail and all seems well here.

[bdraco]

I did a github code search and could not find any use cases of FlowControlDataQueue outside of aiohttp. I think this is safe to partially backport, and it would make it a lot easier to not have to maintain different code in 3.11.

I'm going to tag, it backport it and tweak it to work with 3.11 so it still keeps the same format for the WebSocket messages and uses the tuple to pass the size like it does now

[patchback]

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

❌ Failed to cleanly apply 5241897 on top of patchback/backports/3.11/5241897f6459926028803caaa4782b8ba84f0562/pr-9685

Backporting merged PR #9685 into master

1. Ensure you have a local repo clone of your fork. Unless you cloned it
   from the upstream, this would be your origin remote.
2. 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

3. 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/5241897f6459926028803caaa4782b8ba84f0562/pr-9685 upstream/3.11

4. Now, cherry-pick PR Fix WebSocket reader flow control calculations  #9685 contents into that branch:

   $ git cherry-pick -x 5241897f6459926028803caaa4782b8ba84f0562

   If it'll yell at you with something like fatal: Commit 5241897f6459926028803caaa4782b8ba84f0562 is a merge but no -m option was given., add -m 1 as follows instead:

   $ git cherry-pick -m1 -x 5241897f6459926028803caaa4782b8ba84f0562

5. At this point, you'll probably encounter some merge conflicts. You must
   resolve them in to preserve the patch from PR Fix WebSocket reader flow control calculations  #9685 as close to the
   original as possible.
6. Push this branch to your fork on GitHub:

   $ git push origin patchback/backports/3.11/5241897f6459926028803caaa4782b8ba84f0562/pr-9685

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