Fix websocket connection leak #7978

bdraco · 2023-12-17T21:07:29Z

The existing implementation never closed the transport unless heartbeat was enabled (not default), the pong was not received, and .close() had not been called before the pong was not received since it cancelled the timer.

We now always close the transport after we get the WSMsgType.CLOSE message or an abnormal closure occurs instead of hoping asyncio calls connection_lost which may not happen in a reasonable time frame (or ever?) for a variety of reasons deeper in the stack (network, bad client implementations, etc).

Any time we are closing/setting self._close_code we must either call self.close() or close the transport otherwise its possible to leak the connection.

To facilitate that, and reduce the chance of future refactoring re-introducing the problem, _set_code_close_transport is added which sets the code and closes the transport.

All other places self._close_code is set have been audited to ensure they also call self.close() There is one exception if self._autoclose is set to False and the client closes the connection, than its the responsibility of the caller to close the connection as otherwise it can leak. This is now documented.

fixes #6325

codecov · 2023-12-17T21:25:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5e44ba4) 97.43% compared to head (d0b459d) 97.44%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7978      +/-   ##
==========================================
+ Coverage   97.43%   97.44%   +0.01%     
==========================================
  Files         107      107              
  Lines       32346    32370      +24     
  Branches     3748     3751       +3     
==========================================
+ Hits        31516    31544      +28     
+ Misses        629      624       -5     
- Partials      201      202       +1

Flag	Coverage Δ
CI-GHA	`97.36% <100.00%> (+0.01%)`	⬆️
OS-Linux	`97.03% <100.00%> (+0.01%)`	⬆️
OS-Windows	`95.52% <100.00%> (+<0.01%)`	⬆️
OS-macOS	`96.85% <100.00%> (+0.01%)`	⬆️
Py-3.10.11	`95.44% <100.00%> (+<0.01%)`	⬆️
Py-3.10.13	`96.84% <100.00%> (+0.01%)`	⬆️
Py-3.11.6	`?`
Py-3.11.7	`96.57% <100.00%> (+0.04%)`	⬆️
Py-3.12.0	`96.50% <100.00%> (+0.01%)`	⬆️
Py-3.12.1	`96.62% <100.00%> (+0.01%)`	⬆️
Py-3.8.10	`95.41% <100.00%> (+<0.01%)`	⬆️
Py-3.8.18	`96.77% <100.00%> (+0.01%)`	⬆️
Py-3.9.13	`95.41% <100.00%> (+<0.01%)`	⬆️
Py-3.9.18	`96.80% <100.00%> (+<0.01%)`	⬆️
Py-pypy7.3.13	`96.36% <100.00%> (+0.01%)`	⬆️
VM-macos	`96.85% <100.00%> (+0.01%)`	⬆️
VM-ubuntu	`97.03% <100.00%> (+0.01%)`	⬆️
VM-windows	`95.52% <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.

This reverts commit dd4c3b4.

aiohttp/web_ws.py

tests/test_web_websocket.py

bdraco · 2023-12-18T06:29:00Z

pushed to production a new hours ago. all working well

Dreamsorcerer

I think if the leak in that demo is gone, then lets merge this.

bdraco · 2023-12-18T17:43:05Z

I am no longer able reproduce it with the provided script after this change.

I will deploy the change more widely on my production later today and merge if no regressions are observed

bdraco · 2023-12-18T19:57:14Z

deployed widely. watching logs

bdraco · 2023-12-18T20:42:59Z

Logs look good. Home Assistant's object logger / leak checker doesn't see the leak anymore.

patchback · 2023-12-18T21:27:57Z

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

❌ Failed to cleanly apply 6f1c608 on top of patchback/backports/3.9/6f1c608fe756baa4b82ea8714373cebab0252265/pr-7978

Backporting merged PR #7978 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.9/6f1c608fe756baa4b82ea8714373cebab0252265/pr-7978 upstream/3.9

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

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.9/6f1c608fe756baa4b82ea8714373cebab0252265/pr-7978

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 · 2023-12-18T21:28:04Z

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

❌ Failed to cleanly apply 6f1c608 on top of patchback/backports/3.10/6f1c608fe756baa4b82ea8714373cebab0252265/pr-7978

Backporting merged PR #7978 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/6f1c608fe756baa4b82ea8714373cebab0252265/pr-7978 upstream/3.10

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

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.10/6f1c608fe756baa4b82ea8714373cebab0252265/pr-7978

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.

(cherry picked from commit 6f1c608)

johntdyer · 2024-01-18T19:56:26Z

@Dreamsorcerer any idea when 3.9.2 might be released.... presumably including this change?

Dreamsorcerer · 2024-01-19T23:03:10Z

Will probably come round to it shortly, have been fully focused on a PR for aiohttp-admin for the last month.

spikefishjohn · 2024-02-26T04:00:20Z

Howdy all and @bdraco!

I'm working on a bug report for GNS3 that I think this PR lines up with. The GNS3 issue is that when a web socket client sends a close message the server never closes the tcp socket. I tried the patch above but it doesn't address the issue.

High level GNS3 is calling ws.receive() at which point aiohttp receive() the close message.

I believe the problem is because ws.receive is setting self._closing = True when it reiceves a close message. This then causes self.close() to return here which prevents if msg.type == WSMsgType.CLOSE: from being reached.

I'm almost thinking this should be removed but i'm not sure what the intent of that is so i'm unsure if that is the proper fix.

Should I open a new issue for this?

bdraco · 2024-02-26T04:05:25Z

Howdy all and @bdraco!

I'm working on a bug report for GNS3 that I think this PR lines up with. The GNS3 issue is that when a web socket client sends a close message the server never closes the tcp socket. I tried the patch above but it doesn't address the issue.

High level GNS3 is calling ws.receive() at which point aiohttp receive() the close message.

I believe the problem is because ws.receive is setting self._closing = True when it reiceves a close message. This then causes self.close() to return here which prevents if msg.type == WSMsgType.CLOSE: from being reached.

I'm almost thinking this should be removed but i'm not sure what the intent of that is so i'm unsure if that is the proper fix.

Should I open a new issue for this?

Yes please. Thank you

spikefishjohn · 2024-02-26T04:07:22Z

Will do, thanks for the quick reply.

pxscott · 2024-03-22T18:50:27Z

Hi,

With the changes for this bug, the behavior of asyncio.wait_for(ws.receive(), timeout=1) has changed. Before, this could be used to do a poll for new messages, but now hitting the timeout causes the websocket to be closed. This causes problems in our code, so we are pinning to 3.9.1.

Scott McRae

bdraco · 2024-03-22T19:01:58Z

Hi,

With the changes for this bug, the behavior of asyncio.wait_for(ws.receive(), timeout=1) has changed. Before, this could be used to do a poll for new messages, but now hitting the timeout causes the websocket to be closed. This causes problems in our code, so we are pinning to 3.9.1.

Scott McRae

Use asyncio.wait instead or wrap the receive call in asyncio.shield to prevent cancellation

Dreamsorcerer · 2024-03-25T23:24:53Z

Use asyncio.wait instead or wrap the receive call in asyncio.shield to prevent cancellation

We should probably look at this more closely. The TimeoutError must come from our timeout() call, so that behaviour is probably correct. But, a CancelledError could come from anywhere and maybe shouldn't close the connection. It may be that we need to check if the cancellation came from outside the task (asyncio.current_task().cancelling()) and skip closing otherwise.

More information about this at:
https://superfastpython.com/asyncio-task-cancellation-best-practices/#Practice_1_Do_Not_Consume_CancelledError
And we recently implemented this in aiohttp-sse: https://github.com/aio-libs/aiohttp-sse/pull/459/files

bdraco · 2024-03-26T21:52:26Z

I think the cancellation handling is fine here, but closing the connection when the receive times out/is cancelled is what was not desired.

#8251 will restore the behavior to keep the connection open if receive times out or is cancelled

pxscott · 2024-03-27T18:18:08Z

That's perfect. Thanks!

Dreamsorcerer · 2024-03-29T13:22:16Z

asyncio.wait_for(ws.receive(), timeout=1)

Also, worth pointing out that receive has a timeout parameter, so you don't need wait_for() at all, in fact you're just doubling the number of timeouts being constructed.

pxscott · 2024-03-29T16:58:04Z

Yeah, I noticed that when debugging this. This code was written awhile back, I think before that parameter existed. But changing to use the parameter didn't help with the close logic, so I just pinned the version for now. I'll try using the parameter when i do upgrade once this patch is released.

bdraco added 2 commits December 17, 2023 11:05

Fix websocket connection leak

ada0061

fixes #6325

fix: missed one

56fe7eb

bdraco added backport-3.9 backport-3.10 Trigger automatic backporting to the 3.10 release branch by Patchback robot labels Dec 17, 2023

This was referenced Dec 17, 2023

Too many websocket client disconnect, memory not release #6325

Closed

Reoccuring aiohttp.server error - During handling of the above exception, another exception occurred home-assistant/core#105158

Open

bdraco added 4 commits December 17, 2023 11:42

never let it leak

dd4c3b4

Revert "never let it leak"

94076ca

This reverts commit dd4c3b4.

document autoclose=False leaks

3c90f16

timeline

d47d0d4

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Dec 17, 2023

bdraco commented Dec 17, 2023

View reviewed changes

aiohttp/web_ws.py Show resolved Hide resolved

bdraco added 7 commits December 17, 2023 16:18

coverage

a3f925e

speed up test

a9754ea

speed up test

f14c0bd

speed up test

98564bf

speed up test

43d10be

speed up test

dab76ca

Merge branch 'master' into fix_ws_close

d0b459d

github-advanced-security bot found potential problems Dec 18, 2023

View reviewed changes

tests/test_web_websocket.py Dismissed Show dismissed Hide dismissed

bdraco changed the title ~~DO NOT MERGE: Fix websocket connection leak~~ Fix websocket connection leak Dec 18, 2023

Dreamsorcerer approved these changes Dec 18, 2023

View reviewed changes

bdraco marked this pull request as ready for review December 18, 2023 20:26

bdraco requested review from webknjaz and asvetlov as code owners December 18, 2023 20:26

bdraco merged commit 6f1c608 into master Dec 18, 2023
32 of 34 checks passed

bdraco deleted the fix_ws_close branch December 18, 2023 21:27

bdraco added a commit that referenced this pull request Dec 18, 2023

Fix websocket connection leak (#7978)

8be6aeb

(cherry picked from commit 6f1c608)

bdraco added a commit that referenced this pull request Dec 18, 2023

Fix websocket connection leak (#7978)

e6e30dd

(cherry picked from commit 6f1c608)

bdraco added a commit that referenced this pull request Dec 19, 2023

Fix websocket connection leak (#7978) (#7980)

477b237

bdraco added a commit that referenced this pull request Dec 19, 2023

Fix websocket connection leak (#7978) (#7979)

46f6a90

bdraco mentioned this pull request Feb 13, 2024

The web socket handler is stuck in an undefined state if the client did not close properly #5301

Closed

spikefishjohn mentioned this pull request Feb 20, 2024

WebSocket server does not initiate a TCP Close after receiving WebSocket Close message (RFC 6455 violation) GNS3/gns3-server#2320

Closed

spikefishjohn mentioned this pull request Feb 26, 2024

web_ws - Can't close tcp socket when receiving a close message from client. #8184

Closed

1 task

bdraco mentioned this pull request Mar 26, 2024

Leave websocket transport open if receive times out or is cancelled #8251

Merged

5 tasks

This was referenced Mar 29, 2024

[PR #8251/c21b76d0 backport][3.9] Leave websocket transport open if receive times out or is cancelled #8263

Merged

[PR #8251/c21b76d0 backport][3.10] Leave websocket transport open if receive times out or is cancelled #8264

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix websocket connection leak #7978

Fix websocket connection leak #7978

bdraco commented Dec 17, 2023 •

edited

Loading

codecov bot commented Dec 17, 2023 •

edited

Loading

bdraco commented Dec 18, 2023

Dreamsorcerer left a comment

bdraco commented Dec 18, 2023 •

edited

Loading

bdraco commented Dec 18, 2023

bdraco commented Dec 18, 2023

patchback bot commented Dec 18, 2023 •

edited

Loading

patchback bot commented Dec 18, 2023 •

edited

Loading

johntdyer commented Jan 18, 2024

Dreamsorcerer commented Jan 19, 2024

spikefishjohn commented Feb 26, 2024

bdraco commented Feb 26, 2024

spikefishjohn commented Feb 26, 2024

pxscott commented Mar 22, 2024

bdraco commented Mar 22, 2024 •

edited

Loading

Dreamsorcerer commented Mar 25, 2024

bdraco commented Mar 26, 2024 •

edited

Loading

pxscott commented Mar 27, 2024

Dreamsorcerer commented Mar 29, 2024

pxscott commented Mar 29, 2024

Fix websocket connection leak #7978

Fix websocket connection leak #7978

Conversation

bdraco commented Dec 17, 2023 • edited Loading

codecov bot commented Dec 17, 2023 • edited Loading

Codecov Report

bdraco commented Dec 18, 2023

Dreamsorcerer left a comment

Choose a reason for hiding this comment

bdraco commented Dec 18, 2023 • edited Loading

bdraco commented Dec 18, 2023

bdraco commented Dec 18, 2023

patchback bot commented Dec 18, 2023 • edited Loading

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

Backporting merged PR #7978 into master

patchback bot commented Dec 18, 2023 • edited Loading

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

Backporting merged PR #7978 into master

johntdyer commented Jan 18, 2024

Dreamsorcerer commented Jan 19, 2024

spikefishjohn commented Feb 26, 2024

bdraco commented Feb 26, 2024

spikefishjohn commented Feb 26, 2024

pxscott commented Mar 22, 2024

bdraco commented Mar 22, 2024 • edited Loading

Dreamsorcerer commented Mar 25, 2024

bdraco commented Mar 26, 2024 • edited Loading

pxscott commented Mar 27, 2024

Dreamsorcerer commented Mar 29, 2024

pxscott commented Mar 29, 2024

bdraco commented Dec 17, 2023 •

edited

Loading

codecov bot commented Dec 17, 2023 •

edited

Loading

bdraco commented Dec 18, 2023 •

edited

Loading

patchback bot commented Dec 18, 2023 •

edited

Loading

patchback bot commented Dec 18, 2023 •

edited

Loading

bdraco commented Mar 22, 2024 •

edited

Loading

bdraco commented Mar 26, 2024 •

edited

Loading