Fixes read_timeout on WS connection not respecting ws_connect's timeouts

[arcivanov]

trafficstars

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

• [X] I think the code is well written
• [X] Unit tests for the changes exist
• [ ] Documentation reflects the changes
• [X] 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.
• [X] 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.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[arcivanov]

FYI I'm working on a backport for 3.9.

[bdraco]

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]

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]

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 :smile:

[bdraco]

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.

[arcivanov]

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

[Dreamsorcerer]

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]

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]

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:

e51fb1ff1ebdde566b96af0090c5c63cf1a62b1b c09c538e036619ae549280fb9051fd6084e8252c e898151e4a343425bb383e51f51937cb5d25e1f0 6ab76b084bf5012b7185046162ed92bedcf073b5

[Dreamsorcerer]

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]

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? :smile: I really don't want to vendorize the entire aiohttp fork over this. Unless you're planning on releasing 3.10 this weekend?

[Dreamsorcerer]

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]

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]

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]

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

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

[bdraco]

All good overnight

[patchback[bot]]

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

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

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

4. Now, cherry-pick PR #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
   

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

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

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.

[bdraco]

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

[arcivanov]

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

[bdraco]

You did, I forgot about them https://github.com/aio-libs/aiohttp/pull/8447