Shutdown logic: Only wait on handlers #8495

Dreamsorcerer · 2024-07-13T15:07:18Z

In aiohttp 3.9 we introduced new shutdown logic in run_app() which waited for all pending tasks to complete. This has caused issues with libraries that spawn idle tasks at runtime to wait for connections.

This PR removes that logic and instead just waits on all active request handlers. Documentation will be updated to teach users how to manage tasks themselves.

@bdraco Please confirm this still works correctly with HA. The only difference should be that on application shutdown, active request handlers are now given time to complete instead of being cancelled immediately.

I think an edge case which needs another test is when handler_cancellation is enabled and the handler suppresses the cancellation.

Fixes #8387

for more information, see https://pre-commit.ci

Dreamsorcerer · 2024-07-13T15:09:00Z

aiohttp/web_server.py

@@ -69,6 +72,7 @@ def pre_shutdown(self) -> None:
    async def shutdown(self, timeout: Optional[float] = None) -> None:
        coros = (conn.shutdown(timeout) for conn in self._connections)
        await asyncio.gather(*coros)
+        print("LENGTH", len(self._connections))


I'd like to assert the number of connections at this point, but can't think of a good way to get this in the tests. Any ideas?

Maybe patch the shutdown function and see how many calls are made to it?

I don't think that works. The number of shutdown() calls doesn't matter, it's the number of them which didn't result in the connections being removed from the set as part of the done callback.

I think if I can figure out how to get a reference to the Server object, then I could patch the .clear() call to check how many are still in the set when this is called.

tests/test_run_app.py

bdraco · 2024-07-14T15:40:51Z

Will test later today. Still catching up as I have a house full of family that doesn't have power from the hurricane in Houston

bdraco · 2024-07-14T17:37:07Z

Jul 14 17:35:56 homeassistant homeassistant[569]: 2024-07-14 12:35:56.216 ERROR (MainThread) [homeassistant] Error doing job: Task exception was never retrieved (None)
Jul 14 17:35:56 homeassistant homeassistant[569]: Traceback (most recent call last):
Jul 14 17:35:56 homeassistant homeassistant[569]:   File "/usr/src/homeassistant/homeassistant/components/http/__init__.py", line 243, in stop_server
Jul 14 17:35:56 homeassistant homeassistant[569]:     await server.stop()
Jul 14 17:35:56 homeassistant homeassistant[569]:   File "/usr/src/homeassistant/homeassistant/components/http/__init__.py", line 624, in stop
Jul 14 17:35:56 homeassistant homeassistant[569]:     await self.runner.cleanup()
Jul 14 17:35:56 homeassistant homeassistant[569]:   File "/usr/local/lib/python3.12/site-packages/aiohttp/web_runner.py", line 307, in cleanup
Jul 14 17:35:56 homeassistant homeassistant[569]:     await self._server.shutdown(self._shutdown_timeout)
Jul 14 17:35:56 homeassistant homeassistant[569]:   File "/usr/local/lib/python3.12/site-packages/aiohttp/web_server.py", line 69, in shutdown
Jul 14 17:35:56 homeassistant homeassistant[569]:     await asyncio.gather(*coros)
Jul 14 17:35:56 homeassistant homeassistant[569]:   File "/usr/local/lib/python3.12/site-packages/aiohttp/web_protocol.py", line 269, in shutdown
Jul 14 17:35:56 homeassistant homeassistant[569]:     await self._current_request.wait_for_disconnection()
Jul 14 17:35:56 homeassistant homeassistant[569]:           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Jul 14 17:35:56 homeassistant homeassistant[569]: AttributeError: 'HomeAssistantRequest' object has no attribute 'wait_for_disconnection'

I get the following in HA when I backported this to 3.9 and updated

with the change backported as bdraco@6aa52bd

https://github.com/home-assistant/core/blob/1d62f0e380c9f9bb35a9c5d30293a05526dcbb87/homeassistant/components/http/__init__.py#L271

Dreamsorcerer · 2024-07-14T17:42:36Z

I get the following in HA when I backported this to 3.9 and updated

Seem to be missing a backport of #4200. I assume it subclasses BaseRequest? In which case I expect it will work with that backported.

bdraco · 2024-07-14T17:57:19Z

I'll make another integration branch with that backport to 3.9

I can't test with 3.10 yet until #8482 is backported or I make an integration branch for that. But since HA is on 3.9 better to test with 3.9

bdraco · 2024-07-14T17:59:06Z

#4200 has a whole lot of conflicts so I'll wait for the backport to happen instead of trying to patch it into the integration branch as there is a chance I'll screw it up and give you a bad test result.

Dreamsorcerer · 2024-07-14T23:54:46Z

Should be sorted in #8504.

bdraco · 2024-07-15T01:54:30Z

Tested with HA on 3.9+ this PR + #8504 cherry picked on top

Restarted a few times. Everything seems ok

bdraco · 2024-07-21T15:24:22Z

Still needs a test and changelog, manual functional testing passed.

bdraco · 2024-07-22T14:23:11Z

Adding a skip for now so we can get this into 3.10b0 as Sam is going to do another turn on this to add the test / changelog / docs before the stable release

codecov · 2024-07-22T14:43:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.67%. Comparing base (95897b1) to head (7f303b4).
Report is 2753 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8495      +/-   ##
==========================================
+ Coverage   97.54%   97.67%   +0.13%     
==========================================
  Files         108      107       -1     
  Lines       33549    33286     -263     
  Branches     4027     3918     -109     
==========================================
- Hits        32724    32513     -211     
+ Misses        601      559      -42     
+ Partials      224      214      -10

Flag	Coverage Δ
CI-GHA	`97.59% <100.00%> (+0.14%)`	⬆️
OS-Linux	`97.24% <100.00%> (+0.13%)`	⬆️
OS-Windows	`95.68% <100.00%> (+0.96%)`	⬆️
OS-macOS	`96.91% <100.00%> (+0.13%)`	⬆️
Py-3.10.11	`97.06% <100.00%> (+0.17%)`	⬆️
Py-3.10.14	`97.00% <100.00%> (+0.15%)`	⬆️
Py-3.11.9	`97.22% <100.00%> (+0.15%)`	⬆️
Py-3.12.4	`97.36% <100.00%> (+0.16%)`	⬆️
Py-3.8.10	`95.44% <100.00%> (+0.99%)`	⬆️
Py-3.8.18	`96.89% <100.00%> (+0.15%)`	⬆️
Py-3.9.13	`97.04% <100.00%> (+0.15%)`	⬆️
Py-3.9.19	`96.99% <100.00%> (+0.14%)`	⬆️
Py-pypy7.3.16	`96.56% <100.00%> (+0.14%)`	⬆️
VM-macos	`96.91% <100.00%> (+0.13%)`	⬆️
VM-ubuntu	`97.24% <100.00%> (+0.13%)`	⬆️
VM-windows	`95.68% <100.00%> (+0.96%)`	⬆️

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.

patchback · 2024-07-22T15:09:48Z

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

❌ Failed to cleanly apply 549c95b on top of patchback/backports/3.10/549c95b948dcddd6588f95545ad6c856f693c503/pr-8495

Backporting merged PR #8495 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/549c95b948dcddd6588f95545ad6c856f693c503/pr-8495 upstream/3.10

Now, cherry-pick PR Shutdown logic: Only wait on handlers #8495 contents into that branch:
```
$ git cherry-pick -x 549c95b948dcddd6588f95545ad6c856f693c503
```
If it'll yell at you with something like fatal: Commit 549c95b948dcddd6588f95545ad6c856f693c503 is a merge but no -m option was given., add -m 1 as follows instead:
```
$ git cherry-pick -m1 -x 549c95b948dcddd6588f95545ad6c856f693c503
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR Shutdown logic: Only wait on handlers #8495 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.10/549c95b948dcddd6588f95545ad6c856f693c503/pr-8495

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: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: J. Nick Koston <nick@koston.org> (cherry picked from commit 549c95b)

…ers (#8530) Co-authored-by: pre-commit-ci[bot] Co-authored-by: J. Nick Koston <nick@koston.org> Co-authored-by: Sam Bull <git@sambull.org>

(cherry picked from commit a561fa9)

**This is a backport of PR #8541 as merged into master (a561fa9).** --------- Co-authored-by: Sam Bull <git@sambull.org>

Only wait on handlers

85480eb

Dreamsorcerer added the backport-3.10 Trigger automatic backporting to the 3.10 release branch by Patchback robot label Jul 13, 2024

[pre-commit.ci] auto fixes from pre-commit.com hooks

1457ba5

for more information, see https://pre-commit.ci

Dreamsorcerer commented Jul 13, 2024

View reviewed changes

github-advanced-security bot found potential problems Jul 13, 2024

View reviewed changes

tests/test_run_app.py Dismissed Show dismissed Hide dismissed

Merge branch 'master' into shutdown-handlers-only

1053a80

bdraco approved these changes Jul 21, 2024

View reviewed changes

Merge branch 'master' into shutdown-handlers-only

4141661

bdraco added 3 commits July 21, 2024 10:24

lint

ad4ec81

Merge branch 'master' into shutdown-handlers-only

81f7120

small cleanups to make mergable for beta

5f6b843

bdraco added the bot:chronographer:skip This PR does not need to include a change note label Jul 22, 2024

bdraco approved these changes Jul 22, 2024

View reviewed changes

small cleanups to make mergable for beta

7f303b4

bdraco mentioned this pull request Jul 22, 2024

** 3.10 testing ** #8529

Closed

3 tasks

bdraco marked this pull request as ready for review July 22, 2024 14:32

bdraco requested a review from asvetlov as a code owner July 22, 2024 14:32

bdraco enabled auto-merge (squash) July 22, 2024 14:33

bdraco merged commit 549c95b into master Jul 22, 2024
37 of 38 checks passed

bdraco deleted the shutdown-handlers-only branch July 22, 2024 15:09

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

[PR #8495/549c95b9 backport][3.10] Shutdown logic: Only wait on handl…

6f17a67

…ers (#8530) Co-authored-by: pre-commit-ci[bot] Co-authored-by: J. Nick Koston <nick@koston.org> Co-authored-by: Sam Bull <git@sambull.org>

Dreamsorcerer added a commit that referenced this pull request Jul 29, 2024

Cleanup for #8495 (#8541)

a561fa9

patchback bot pushed a commit that referenced this pull request Jul 29, 2024

Cleanup for #8495 (#8541)

7559c75

(cherry picked from commit a561fa9)

Dreamsorcerer added a commit that referenced this pull request Jul 29, 2024

[PR #8541/a561fa99 backport][3.10] Cleanup for #8495 (#8544)

ed8de3a

**This is a backport of PR #8541 as merged into master (a561fa9).** --------- Co-authored-by: Sam Bull <git@sambull.org>

Dreamsorcerer mentioned this pull request Aug 5, 2024

Fix slowdown in tests waiting for server to close #8609

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shutdown logic: Only wait on handlers #8495

Shutdown logic: Only wait on handlers #8495

Dreamsorcerer commented Jul 13, 2024 •

edited

Loading

Dreamsorcerer Jul 13, 2024

bdraco Jul 15, 2024 •

edited

Loading

Dreamsorcerer Jul 27, 2024

Dreamsorcerer Jul 27, 2024

bdraco commented Jul 14, 2024

bdraco commented Jul 14, 2024 •

edited

Loading

Dreamsorcerer commented Jul 14, 2024

bdraco commented Jul 14, 2024

bdraco commented Jul 14, 2024

Dreamsorcerer commented Jul 14, 2024

bdraco commented Jul 15, 2024

bdraco commented Jul 21, 2024

bdraco commented Jul 22, 2024 •

edited

Loading

codecov bot commented Jul 22, 2024 •

edited

Loading

patchback bot commented Jul 22, 2024 •

edited

Loading

Shutdown logic: Only wait on handlers #8495

Shutdown logic: Only wait on handlers #8495

Conversation

Dreamsorcerer commented Jul 13, 2024 • edited Loading

Dreamsorcerer Jul 13, 2024

Choose a reason for hiding this comment

bdraco Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Dreamsorcerer Jul 27, 2024

Choose a reason for hiding this comment

Dreamsorcerer Jul 27, 2024

Choose a reason for hiding this comment

bdraco commented Jul 14, 2024

bdraco commented Jul 14, 2024 • edited Loading

Dreamsorcerer commented Jul 14, 2024

bdraco commented Jul 14, 2024

bdraco commented Jul 14, 2024

Dreamsorcerer commented Jul 14, 2024

bdraco commented Jul 15, 2024

bdraco commented Jul 21, 2024

bdraco commented Jul 22, 2024 • edited Loading

codecov bot commented Jul 22, 2024 • edited Loading

Codecov Report

patchback bot commented Jul 22, 2024 • edited Loading

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

Backporting merged PR #8495 into master

Dreamsorcerer commented Jul 13, 2024 •

edited

Loading

bdraco Jul 15, 2024 •

edited

Loading

bdraco commented Jul 14, 2024 •

edited

Loading

bdraco commented Jul 22, 2024 •

edited

Loading

codecov bot commented Jul 22, 2024 •

edited

Loading

patchback bot commented Jul 22, 2024 •

edited

Loading