Fixes read_timeout on WS connection not respecting ws_connect's timeouts by arcivanov · Pull Request #8445 · aio-libs/aiohttp

arcivanov · 2024-06-07T20:55:34Z

Added read_timeout property to ResponseHandler to allow override

After WS(S) connection is established, adjust conn.proto.read_timeout to
be the largest of the read_timeout and the ws_connect's
timeout.ws_receive with None treated as 'no timeout', i.e. the maximum.

fixes #8444

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_or_pr_num>.<type>.rst (e.g. 588.bugfix.rst)
- if you don't have an issue number, change it to the pull request
  number after creating the PR
  - .bugfix: A bug fix for something the maintainers deemed an
    improper undesired behavior that got corrected to match
    pre-agreed expectations.
  - .feature: A new behavior, public APIs. That sort of stuff.
  - .deprecation: A declaration of future API removals and breaking
    changes in behavior.
  - .breaking: When something public is removed in a breaking way.
    Could be deprecated in an earlier release.
  - .doc: Notable updates to the documentation structure or build
    process.
  - .packaging: Notes for downstreams about unobvious side effects
    and tooling. Changes in the test invocation considerations and
    runtime assumptions.
  - .contrib: Stuff that affects the contributor experience. e.g.
    Running tests, building the docs, setting up the development
    environment.
  - .misc: Changes that are hard to assign to any of the above
    categories.
- Make sure to use full sentences with correct case and punctuation,
  for example:
```
Fixed issue with non-ascii contents in doctest text files
-- by :user:`contributor-gh-handle`.
```
  Use the past tense or the present tense a non-imperative mood,
  referring to what's changed compared to the last released version
  of this project.

Please see aio-libs#8445 for the source PR

codecov · 2024-06-07T23:01:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.62%. Comparing base (7bf6ee1) to head (ce0bea6).
Report is 920 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #8445   +/-   ##
=======================================
  Coverage   97.61%   97.62%           
=======================================
  Files         107      107           
  Lines       33159    33230   +71     
  Branches     3895     3902    +7     
=======================================
+ Hits        32369    32440   +71     
  Misses        571      571           
  Partials      219      219

Flag	Coverage Δ
CI-GHA	`97.53% <100.00%> (+<0.01%)`	⬆️
OS-Linux	`97.20% <100.00%> (+<0.01%)`	⬆️
OS-Windows	`95.66% <100.00%> (+<0.01%)`	⬆️
OS-macOS	`96.86% <100.00%> (+<0.01%)`	⬆️
Py-3.10.11	`97.01% <100.00%> (+<0.01%)`	⬆️
Py-3.10.14	`96.96% <100.00%> (+<0.01%)`	⬆️
Py-3.11.9	`97.18% <100.00%> (+<0.01%)`	⬆️
Py-3.12.4	`97.31% <100.00%> (+<0.01%)`	⬆️
Py-3.8.10	`95.43% <100.00%> (+<0.01%)`	⬆️
Py-3.8.18	`96.85% <100.00%> (+<0.01%)`	⬆️
Py-3.9.13	`96.99% <100.00%> (+<0.01%)`	⬆️
Py-3.9.19	`96.95% <100.00%> (+<0.01%)`	⬆️
Py-pypy7.3.16	`96.52% <100.00%> (+<0.01%)`	⬆️
VM-macos	`96.86% <100.00%> (+<0.01%)`	⬆️
VM-ubuntu	`97.20% <100.00%> (+<0.01%)`	⬆️
VM-windows	`95.66% <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.

aiohttp/client.py

aiohttp/client_proto.py

tests/test_client_ws.py

arcivanov · 2024-06-08T00:06:00Z

FYI I'm working on a backport for 3.9.

Please see aio-libs#8445 for the source PR

tests/test_client_ws.py

Added read_timeout property to ResponseHandler to allow override After WS(S) connection is established, adjust `conn.proto.read_timeout` to be the largest of the `read_timeout` and the `ws_connect`'s `timeout.ws_receive` with `None` treated as 'no timeout', i.e. the maximum. fixes aio-libs#8444

Please see aio-libs#8445 for the source PR

bdraco · 2024-06-08T00:48:55Z

We will need to backport to 3.10 as well as I'm not sure if we plan on doing any more 3.9 releases before 3.10 starts rolling out.

arcivanov · 2024-06-08T00:54:28Z

I want to leave this note here for whoever comes looking.

Between 3.9 and master (f662958) there was a change in the interpretation of WS timeouts.
receive_timeout and timeout in 3.9 became ws_timeout.ws_receive and ws_timeout.ws_close
As such this solution only looks at receive_timeout in 3.9 and ws_timeout.ws_receive in master for the purposes of setting the conn_proto.read_timeout.

Not considering timeout in 3.9, as I originally planned, will remove unexpected changes in behavior in 4.x when timeout becomes used for WS closing timeout only.

@bdraco

arcivanov · 2024-06-08T00:55:08Z

We will need to backport to 3.10 as well as I'm not sure if we plan on doing any more 3.9 releases before 3.10 starts rolling out.

@bdraco
Could I perhaps request a hotfix release with this for 3.9? If you oblige, I'll backport to 3.10 right now 😄

aiohttp/client.py

bdraco · 2024-06-08T01:02:53Z

We will need to backport to 3.10 as well as I'm not sure if we plan on doing any more 3.9 releases before 3.10 starts rolling out.

@bdraco Could I perhaps request a hotfix release with this for 3.9? If you oblige, I'll backport to 3.10 right now 😄

That's one I can't answer as I've not done a release for this project before (only reviews + code) so I'll defer to other members to answer.

… not respecting ws_connect's timeouts aio-libs#8444 Please see aio-libs#8445 for the source PR

arcivanov · 2024-06-08T01:06:35Z

@bdraco @Dreamsorcerer posted 3.10 backport as well. It's virtually identical to 3.9.

Dreamsorcerer · 2024-06-08T01:06:52Z

Between 3.9 and master (f662958) there was a change in the interpretation of WS timeouts.

I think that may be a missing backport. Seems sensible that the change should have been backported, same as the .request() method now uses ClientTimeout.

bdraco · 2024-06-08T01:07:14Z

I don't expect any regressions, but just to be safe I'll push the 3.9 backport to a few production Home Assistant instances and run it overnight to test

arcivanov · 2024-06-08T01:10:37Z

Between 3.9 and master (f662958) there was a change in the interpretation of WS timeouts.

I think that may be a missing backport. Seems sensible that the change should have been backported, same as the .request() method now uses ClientTimeout.

I don't think so. That's an API change and it was posted in 2019/0. It introduces new user-facing timeout data structures in public API:

e51fb1f
c09c538
e898151
6ab76b0

Dreamsorcerer · 2024-06-08T01:19:33Z

That's one I can't answer as I've not done a release for this project before (only reviews + code) so I'll defer to other members to answer.

I'm not planning to do another 3.9 release unless there's a notable security issue (but, I think we can plan to do the 3.10 release soon if we want).

arcivanov · 2024-06-08T01:21:40Z

That's one I can't answer as I've not done a release for this project before (only reviews + code) so I'll defer to other members to answer.

I'm not planning to do another 3.9 release unless there's a notable security issue (but, I think we can plan to do the 3.10 release soon if we want).

Pleeeeeeeease? 😄 I really don't want to vendorize the entire aiohttp fork over this. Unless you're planning on releasing 3.10 this weekend?

Dreamsorcerer · 2024-06-08T01:25:32Z

I don't think so. That's an API change and it was posted in 2019/0. It introduces new user-facing timeout data structures in public API:

Yes, looks like the same change as ClientTimeout. The old parameters are deprecated, but still maintained for backwards compatibility in 3.x. I suspect the same should have been done for this. There are a lot of old PRs which missed backports by mistake (you'll see the PRs have no backport labels at all). It's why I added a CI check that backport labels are added to every PR, so we can be sure whether a PR was intended to be backported or not.

Dreamsorcerer · 2024-06-08T01:27:28Z

That's one I can't answer as I've not done a release for this project before (only reviews + code) so I'll defer to other members to answer.

I'm not planning to do another 3.9 release unless there's a notable security issue (but, I think we can plan to do the 3.10 release soon if we want).

Pleeeeeeeease? 😄 I really don't want to vendorize the entire aiohttp fork over this. Unless you're planning on releasing 3.10 this weekend?

I'm not doing any release this weekend. :P
The amount of work is probably less to release 3.10 than to do another 3.9 release.

arcivanov · 2024-06-08T01:36:03Z

That's one I can't answer as I've not done a release for this project before (only reviews + code) so I'll defer to other members to answer.

I'm not planning to do another 3.9 release unless there's a notable security issue (but, I think we can plan to do the 3.10 release soon if we want).

Pleeeeeeeease? 😄 I really don't want to vendorize the entire aiohttp fork over this. Unless you're planning on releasing 3.10 this weekend?

I'm not doing any release this weekend. :P The amount of work is probably less to release 3.10 than to do another 3.9 release.

Well, I had to try ;)
Anyway, I've PRed 3.9 and 3.10 backports preserving the existing APIs. In the master the fix utilizes the new ClientWSTimeout APIs so you have both options.

Now I have to go patch the installed library at runtime to make sure this fix gets in.

bdraco · 2024-06-08T01:37:10Z

Pushed to a few production Home Assistant instances. Nothing blew up.

Will report back in the morning before I'm traveling again.

bdraco · 2024-06-08T19:31:53Z

All good overnight

patchback · 2024-07-17T13:44:04Z

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

❌ Failed to cleanly apply b860848 on top of patchback/backports/3.10/b860848ab49177f98ac91cf6faa16037fc99bef2/pr-8445

Backporting merged PR #8445 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/b860848ab49177f98ac91cf6faa16037fc99bef2/pr-8445 upstream/3.10

Now, cherry-pick PR Fixes read_timeout on WS connection not respecting ws_connect's timeouts #8445 contents into that branch:
```
$ git cherry-pick -x b860848ab49177f98ac91cf6faa16037fc99bef2
```
If it'll yell at you with something like fatal: Commit b860848ab49177f98ac91cf6faa16037fc99bef2 is a merge but no -m option was given., add -m 1 as follows instead:
```
$ git cherry-pick -m1 -x b860848ab49177f98ac91cf6faa16037fc99bef2
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR Fixes read_timeout on WS connection not respecting ws_connect's timeouts #8445 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.10/b860848ab49177f98ac91cf6faa16037fc99bef2/pr-8445

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.

bdraco · 2024-07-17T13:44:35Z

@arcivanov We are getting closer to releasing 3.10. Would you kindly take care of the backport?

arcivanov · 2024-07-17T15:31:28Z

@bdraco I was under impression I already did. Is it conflicting? I'll confirm in a couple of hours.

bdraco · 2024-07-17T15:51:38Z

You did, I forgot about them #8447

…pecting ws_connect's timeouts #8444 (#8447) Co-authored-by: J. Nick Koston <[email protected]>

arcivanov requested a review from asvetlov as a code owner June 7, 2024 20:55

arcivanov force-pushed the issue_8444 branch from a1cd0ae to 24a8728 Compare June 7, 2024 21:04

arcivanov requested a review from webknjaz as a code owner June 7, 2024 21:04

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

arcivanov force-pushed the issue_8444 branch 3 times, most recently from 2bb2387 to 72bb110 Compare June 7, 2024 22:01

arcivanov added a commit to arcivanov/aiohttp that referenced this pull request Jun 7, 2024

Backport: aio-libs#8444 into 3.9

651b4af

Please see aio-libs#8445 for the source PR

arcivanov mentioned this pull request Jun 7, 2024

[PR #8445 backport][3.9] Fixes read_timeout on WS connection not respecting ws_connect's timeouts #8444 #8446

Closed

arcivanov force-pushed the issue_8444 branch 2 times, most recently from 11b2682 to 28f126c Compare June 7, 2024 22:52

bdraco added backport-3.9 labels Jun 7, 2024

bdraco reviewed Jun 7, 2024

View reviewed changes

aiohttp/client.py Outdated Show resolved Hide resolved

bdraco reviewed Jun 7, 2024

View reviewed changes

aiohttp/client_proto.py Show resolved Hide resolved

bdraco reviewed Jun 7, 2024

View reviewed changes

tests/test_client_ws.py Show resolved Hide resolved

arcivanov added a commit to arcivanov/aiohttp that referenced this pull request Jun 8, 2024

Backport: aio-libs#8444 into 3.9

05cb37f

Please see aio-libs#8445 for the source PR

bdraco reviewed Jun 8, 2024

View reviewed changes

tests/test_client_ws.py Outdated Show resolved Hide resolved

arcivanov force-pushed the issue_8444 branch 3 times, most recently from b74ddd7 to f96e11a Compare June 8, 2024 00:40

arcivanov changed the title ~~Fixes socket timeout on WS connection not respecting ws_connect's timeouts~~ Fixes read_timeout on WS connection not respecting ws_connect's timeouts Jun 8, 2024

arcivanov force-pushed the issue_8444 branch from f96e11a to 6083b21 Compare June 8, 2024 00:41

arcivanov added a commit to arcivanov/aiohttp that referenced this pull request Jun 8, 2024

Backport: aio-libs#8444 into 3.9

6b2a182

Please see aio-libs#8445 for the source PR

arcivanov added a commit to arcivanov/aiohttp that referenced this pull request Jun 8, 2024

Backport: aio-libs#8444 into 3.9

db19243

Please see aio-libs#8445 for the source PR

Dreamsorcerer reviewed Jun 8, 2024

View reviewed changes

aiohttp/client.py Show resolved Hide resolved

arcivanov added a commit to arcivanov/aiohttp that referenced this pull request Jun 8, 2024

[PR aio-libs#8445 backport][3.10] Fixes read_timeout on WS connection…

ce5bcb2

… not respecting ws_connect's timeouts aio-libs#8444 Please see aio-libs#8445 for the source PR

arcivanov mentioned this pull request Jun 8, 2024

[PR #8445 backport][3.10] Fixes read_timeout on WS connection not respecting ws_connect's timeouts #8444 #8447

Merged

bdraco approved these changes Jun 8, 2024

View reviewed changes

Merge branch 'master' into issue_8444

ce0bea6

bdraco removed the backport-3.9 label Jul 15, 2024

bdraco approved these changes Jul 17, 2024

View reviewed changes

bdraco merged commit b860848 into aio-libs:master Jul 17, 2024

bdraco added a commit that referenced this pull request Jul 17, 2024

[PR #8445 backport][3.10] Fixes read_timeout on WS connection not res…

776ebc6

…pecting ws_connect's timeouts #8444 (#8447) Co-authored-by: J. Nick Koston <[email protected]>

Uh oh!

Conversation

arcivanov commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

codecov bot commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arcivanov commented Jun 8, 2024

Uh oh!

Uh oh!

bdraco commented Jun 8, 2024

Uh oh!

arcivanov commented Jun 8, 2024

Uh oh!

arcivanov commented Jun 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

bdraco commented Jun 8, 2024

Uh oh!

arcivanov commented Jun 8, 2024

Uh oh!

Dreamsorcerer commented Jun 8, 2024

Uh oh!

bdraco commented Jun 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arcivanov commented Jun 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dreamsorcerer commented Jun 8, 2024

Uh oh!

arcivanov commented Jun 8, 2024

Uh oh!

Dreamsorcerer commented Jun 8, 2024

Uh oh!

Dreamsorcerer commented Jun 8, 2024

Uh oh!

arcivanov commented Jun 8, 2024

Uh oh!

bdraco commented Jun 8, 2024

Uh oh!

bdraco commented Jun 8, 2024

Uh oh!

patchback bot commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Backporting merged PR #8445 into master

Uh oh!

bdraco commented Jul 17, 2024

Uh oh!

arcivanov commented Jul 17, 2024

Uh oh!

bdraco commented Jul 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

arcivanov commented Jun 7, 2024 •

edited

Loading

codecov bot commented Jun 7, 2024 •

edited

Loading

arcivanov commented Jun 8, 2024 •

edited

Loading

bdraco commented Jun 8, 2024 •

edited

Loading

arcivanov commented Jun 8, 2024 •

edited

Loading

patchback bot commented Jul 17, 2024 •

edited

Loading