Discard non-close messages in close timeout window #9508

lenard-mosys · 2024-10-18T14:32:50Z

What do these changes do?

Waits for a close message in the close timeout window, not just the first message.

Are there changes in behavior for the user?

Previously spurious abnormally closed websocket connections could be reported to the server, even if the client responded to the close message with a close in a timely manner.

Is it a substantial burden for the maintainers to support this?

No

Related issue number

#9506

Checklist

I think the code is well written
Unit tests for the changes exist
~~Documentation reflects the changes~~
- It is a bugfix.
If you provide code modification, please add yourself to CONTRIBUTORS.txt
Add a new news fragment into the CHANGES/ folder

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

lenard-mosys · 2024-10-18T15:44:41Z

Breaks this test:

aiohttp/tests/test_web_websocket_functional.py

Lines 1235 to 1254 in 9e975d2

    
           async def test_abnormal_closure_when_server_does_not_receive( 
        
               aiohttp_client: AiohttpClient, 
        
           ) -> None: 
        
               """Test abnormal closure when the server closes and a message is pending.""" 
        
               async def handler(request: web.Request) -> web.WebSocketResponse: 
        
                   ws = web.WebSocketResponse() 
        
                   await ws.prepare(request) 
        
                   await ws.close() 
        
                   return ws 
        
               app = web.Application() 
        
               app.router.add_route("GET", "/", handler) 
        
               client = await aiohttp_client(app) 
        
               resp = await client.ws_connect("/") 
        
               await resp.send_str("some data") 
        
               await asyncio.sleep(0.1) 
        
               msg = await resp.receive() 
        
               assert msg.type is aiohttp.WSMsgType.CLOSE 
        
               assert resp.close_code == WSCloseCode.ABNORMAL_CLOSURE

IMO the test is broken, or at least names and comments are misleading. I'm a bit perplexed why this test even passed before. Note that the test tests for the close code of the ClientWebSocketResponse, not for the server's WebSocketResponse. The close code for ws within the handler should indeed be abnormal closure, but that's never tested.

I'm not entirely sure what this test intends to test, or even if it intends to test server or client side behavior.

bdraco · 2024-10-18T15:57:07Z

I'm not entirely sure what this test intends to test, or even if it intends to test server or client side behavior.

The intent of the test is to verify the client gets an ABNORMAL_CLOSURE when sever initiated close while it was trying to send a str

lenard-mosys · 2024-10-18T16:22:30Z

The intent of the test is to verify the client gets an ABNORMAL_CLOSURE when sever initiated close while it was trying to send a str

I think this is questionable. The server closed the connection cleanly, even if the client tried to send a message. There could be connection schemes where the client sends fire-and-forget status updates, and the server could close the connection whenever. The client should get the close code sent from the server, not abnormal closure in this case.

Having said that I struggle to find normative wording in the RFC that would describe the conforming behavior in this situation. The best I can find is (under "Normal Closure of Connections"):

https://datatracker.ietf.org/doc/html/rfc6455#section-7.3

Servers MAY close the WebSocket connection whenever desired.

And for 1006:

It is designated for use in applications expecting a status code to indicate that the connection was closed abnormally, e.g., without sending or receiving a Close control frame.

The "e.g." doesn't help here, suggesting that there are other situations where this is applicable.

Anyway, I think there are some problems with the autoclose mechanism of the client, and I can make a similar test fail to have the expected result, even when the client doesn't try to send a message that the server doesn't expect.

bdraco · 2024-10-18T16:27:09Z

Your changes seems reasonable to me. We likely need to adjust the test. I agree the RFC is not crystal clear.

We also need a test for the new behavior as well

lenard-mosys · 2024-10-18T16:33:47Z

Your changes seems reasonable to me. We likely need to adjust the test. I agree the RFC is not crystal clear.

In any case, there is probably a missing test that checks for the server side behavior. This particular test can be made to pass with ws = web.WebSocketResponse(timeout=0.), which restores (roughly) the server-side behavior the client can test against, but I don't know how palatable that is.

bdraco · 2024-10-18T16:48:10Z

Your changes seems reasonable to me. We likely need to adjust the test. I agree the RFC is not crystal clear.

In any case, there is probably a missing test that checks for the server side behavior. This particular test can be made to pass with ws = web.WebSocketResponse(timeout=0.), which restores (roughly) the server-side behavior the client can test against, but I don't know how palatable that is.

I think thats fine, but please add a comment to explain the 0 timeout.

codecov · 2024-10-21T20:34:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.67%. Comparing base (75ae623) to head (2d750ac).
Report is 4 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #9508   +/-   ##
=======================================
  Coverage   98.66%   98.67%           
=======================================
  Files         116      116           
  Lines       35638    35676   +38     
  Branches     4225     4229    +4     
=======================================
+ Hits        35164    35202   +38     
  Misses        319      319           
  Partials      155      155

Flag	Coverage Δ
CI-GHA	`98.55% <100.00%> (+<0.01%)`	⬆️
OS-Linux	`98.23% <100.00%> (+<0.01%)`	⬆️
OS-Windows	`96.12% <100.00%> (+<0.01%)`	⬆️
OS-macOS	`97.43% <100.00%> (+<0.01%)`	⬆️
Py-3.10.11	`97.28% <100.00%> (+1.60%)`	⬆️
Py-3.10.15	`97.71% <100.00%> (+<0.01%)`	⬆️
Py-3.11.10	`97.76% <93.02%> (-0.01%)`	⬇️
Py-3.11.9	`97.35% <93.02%> (-0.02%)`	⬇️
Py-3.12.7	`98.25% <100.00%> (+<0.01%)`	⬆️
Py-3.13.0	`98.24% <100.00%> (+<0.01%)`	⬆️
Py-3.9.13	`97.20% <100.00%> (+<0.01%)`	⬆️
Py-3.9.20	`97.62% <100.00%> (+<0.01%)`	⬆️
Py-pypy7.3.16	`97.25% <100.00%> (+<0.01%)`	⬆️
VM-macos	`97.43% <100.00%> (+<0.01%)`	⬆️
VM-ubuntu	`98.23% <100.00%> (+<0.01%)`	⬆️
VM-windows	`96.12% <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.

bdraco · 2024-10-28T23:39:57Z

@lenard-mosys It looks like a conflict has appeared

…lose_wait

codspeed-hq · 2024-10-29T14:54:12Z

CodSpeed Performance Report

Merging #9508 will not alter performance

_{Comparing lenard-mosys:websocketresponse_close_wait (2d750ac) with master (3f2f4a7)}

Summary

✅ 13 untouched benchmarks

CHANGES/9506.bugfix.rst

aiohttp/web_ws.py

tests/test_web_websocket_functional.py

Co-authored-by: J. Nick Koston <[email protected]>

bdraco · 2024-11-05T14:56:56Z

@lenard-mosys We are likely doing another release soon. Not sure how keen you are to get this in the next one, but just in case you missed my comment above about the missing branch coverage for https://github.com/aio-libs/aiohttp/pull/9508/files#diff-37008174f47abcf385c9bd15d713b586a921dc6aad7d75acd1510aebc75d2b4fR503

lenard-mosys · 2024-11-05T16:45:12Z

@lenard-mosys We are likely doing another release soon. Not sure how keen you are to get this in the next one, but just in case you missed my comment above about the missing branch coverage for https://github.com/aio-libs/aiohttp/pull/9508/files#diff-37008174f47abcf385c9bd15d713b586a921dc6aad7d75acd1510aebc75d2b4fR503

I'm not in a hurry with this. I added a test that hopefully covers the previously missing branch.

…lose_wait

bdraco · 2024-11-05T16:47:07Z

Thanks. I'll take a look as soon as I can.

CI is a bit busy now as I'm cleaning up to prep another beta release

bdraco · 2024-11-05T17:13:52Z

Looks like that branch is still coming up on the coverage report

bdraco · 2024-11-05T17:14:23Z

https://app.codecov.io/gh/aio-libs/aiohttp/pull/9508/blob/aiohttp/web_ws.py

lenard-mosys · 2024-11-05T17:18:36Z

Looks like that branch is still coming up on the coverage report

Weird. I don't understand how the next line is covered then. My added test should cover the hidden else case here. I guess I could just add an else: pass and see what happens. Anyway, I don't have more time today for this, I will follow up tomorrow.

lenard-mosys · 2024-11-06T11:00:18Z

I have some trouble covering the missing branch (that is producing a WsMsgType.ERROR message from await reader.read() within WebSocketResponse.close()). I tried sending an oversized message from the client side, with setting WebSocketResponse(max_msg_size=8) on the server size, but that produces an exception instead from await reader.read().

It looks like that WsMsgType.ERROR is produced by exceptions, but those exceptions get reraised anyway from await reader.read(), therefore it's impossible to get this type of message at this level.

bdraco · 2024-11-06T18:35:55Z

It looks like the only place we could get one is from

    def _handle_ping_pong_exception(self, exc: BaseException) -> None:
        """Handle exceptions raised during ping/pong processing."""
        if self._closed:
            return
        self._set_closed()
        self._set_code_close_transport(WSCloseCode.ABNORMAL_CLOSURE)
        self._exception = exc
        if self._waiting and not self._closing and self._reader is not None:
            self._reader.feed_data(WSMessageError(data=exc, extra=None))

bdraco · 2024-11-06T18:36:57Z

For that to happen, close would have to be called right after _handle_ping_pong_exception happened

bdraco · 2024-11-06T18:41:56Z

But that will set self._closed = True

bdraco · 2024-11-06T18:42:04Z

So I agree its truly unreachable.

aiohttp/web_ws.py

bdraco · 2024-11-06T18:45:11Z

Thanks @lenard-mosys !

CHANGES/9506.bugfix.rst

patchback · 2024-11-06T19:41:23Z

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

❌ Failed to cleanly apply 274c54e on top of patchback/backports/3.10/274c54e46c11e6f527326a3574eb984f90e0f740/pr-9508

Backporting merged PR #9508 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/274c54e46c11e6f527326a3574eb984f90e0f740/pr-9508 upstream/3.10

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

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.10/274c54e46c11e6f527326a3574eb984f90e0f740/pr-9508

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 · 2024-11-06T19:41:30Z

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

❌ Failed to cleanly apply 274c54e on top of patchback/backports/3.11/274c54e46c11e6f527326a3574eb984f90e0f740/pr-9508

Backporting merged PR #9508 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.11/274c54e46c11e6f527326a3574eb984f90e0f740/pr-9508 upstream/3.11

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

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.11/274c54e46c11e6f527326a3574eb984f90e0f740/pr-9508

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 <[email protected]> (cherry picked from commit 274c54e)

bdraco · 2024-11-06T19:49:41Z

3.10 #9687

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: J. Nick Koston <[email protected]> (cherry picked from commit 274c54e)

bdraco · 2024-11-06T19:50:42Z

3.11 #9688

… timeout window (#9688) Co-authored-by: pre-commit-ci[bot] Co-authored-by: J. Nick Koston <[email protected]> Co-authored-by: lenard-mosys <[email protected]>

… timeout window (#9687) Co-authored-by: pre-commit-ci[bot] Co-authored-by: J. Nick Koston <[email protected]> Co-authored-by: lenard-mosys <[email protected]>

Discard non-close messages in close timeout window

9e975d2

lenard-mosys requested a review from asvetlov as a code owner October 18, 2024 14:32

Add to CHANGES and CONTRIBUTORS

f807cb0

lenard-mosys requested a review from webknjaz as a code owner October 18, 2024 14:41

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

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

8e803d0

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

Set close timeout to 0 in abnormal closure test

476e37f

bdraco added backport-3.10 Trigger automatic backporting to the 3.10 release branch by Patchback robot backport-3.11 Trigger automatic backporting to the 3.11 release branch by Patchback robot labels Oct 21, 2024

lenard-mosys added 3 commits October 21, 2024 22:09

Test abnormal closure when client does not respond to close

4e3b283

Fix formatting and linting errors

a063ac0

Simplify WebSocketResponse.close(), remove dead code

e0b8dee

Merge remote-tracking branch 'origin/master' into websocketresponse_c…

1fef4a0

…lose_wait

bdraco reviewed Oct 29, 2024

View reviewed changes

CHANGES/9506.bugfix.rst Outdated Show resolved Hide resolved

bdraco reviewed Oct 29, 2024

View reviewed changes

aiohttp/web_ws.py Show resolved Hide resolved

bdraco reviewed Oct 29, 2024

View reviewed changes

tests/test_web_websocket_functional.py Outdated Show resolved Hide resolved

lenard-mosys and others added 3 commits October 31, 2024 13:21

Update CHANGES/9506.bugfix.rst

ac7f901

Co-authored-by: J. Nick Koston <[email protected]>

Handle WsMsgType.ERROR message during close() timeout window

130cf38

Change branch to assert

c7990ee

Add test for normal closure while client sends message

c54dfee

Merge remote-tracking branch 'origin/master' into websocketresponse_c…

0ebcfe1

…lose_wait

bdraco reviewed Nov 6, 2024

View reviewed changes

aiohttp/web_ws.py Outdated Show resolved Hide resolved

Update aiohttp/web_ws.py

5d1279a

bdraco approved these changes Nov 6, 2024

View reviewed changes

bdraco reviewed Nov 6, 2024

View reviewed changes

CHANGES/9506.bugfix.rst Outdated Show resolved Hide resolved

Cleanup changelog message to link to method

2d750ac

bdraco approved these changes Nov 6, 2024

View reviewed changes

bdraco enabled auto-merge (squash) November 6, 2024 19:22

bdraco merged commit 274c54e into aio-libs:master Nov 6, 2024
37 of 38 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Discard non-close messages in close timeout window #9508

Discard non-close messages in close timeout window #9508

lenard-mosys commented Oct 18, 2024 •

edited

Loading

lenard-mosys commented Oct 18, 2024

bdraco commented Oct 18, 2024

lenard-mosys commented Oct 18, 2024

bdraco commented Oct 18, 2024 •

edited

Loading

lenard-mosys commented Oct 18, 2024

bdraco commented Oct 18, 2024

codecov bot commented Oct 21, 2024 •

edited

Loading

bdraco commented Oct 28, 2024

codspeed-hq bot commented Oct 29, 2024 •

edited

Loading

bdraco commented Nov 5, 2024

lenard-mosys commented Nov 5, 2024

bdraco commented Nov 5, 2024

bdraco commented Nov 5, 2024

bdraco commented Nov 5, 2024

lenard-mosys commented Nov 5, 2024

lenard-mosys commented Nov 6, 2024

bdraco commented Nov 6, 2024

bdraco commented Nov 6, 2024

bdraco commented Nov 6, 2024

bdraco commented Nov 6, 2024

bdraco commented Nov 6, 2024

patchback bot commented Nov 6, 2024 •

edited

Loading

patchback bot commented Nov 6, 2024 •

edited

Loading

bdraco commented Nov 6, 2024

bdraco commented Nov 6, 2024

Discard non-close messages in close timeout window #9508

Discard non-close messages in close timeout window #9508

Conversation

lenard-mosys commented Oct 18, 2024 • edited Loading

What do these changes do?

Are there changes in behavior for the user?

Is it a substantial burden for the maintainers to support this?

Related issue number

Checklist

lenard-mosys commented Oct 18, 2024

bdraco commented Oct 18, 2024

lenard-mosys commented Oct 18, 2024

bdraco commented Oct 18, 2024 • edited Loading

lenard-mosys commented Oct 18, 2024

bdraco commented Oct 18, 2024

codecov bot commented Oct 21, 2024 • edited Loading

Codecov Report

bdraco commented Oct 28, 2024

codspeed-hq bot commented Oct 29, 2024 • edited Loading

CodSpeed Performance Report

Merging #9508 will not alter performance

Summary

bdraco commented Nov 5, 2024

lenard-mosys commented Nov 5, 2024

bdraco commented Nov 5, 2024

bdraco commented Nov 5, 2024

bdraco commented Nov 5, 2024

lenard-mosys commented Nov 5, 2024

lenard-mosys commented Nov 6, 2024

bdraco commented Nov 6, 2024

bdraco commented Nov 6, 2024

bdraco commented Nov 6, 2024

bdraco commented Nov 6, 2024

bdraco commented Nov 6, 2024

patchback bot commented Nov 6, 2024 • edited Loading

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

Backporting merged PR #9508 into master

patchback bot commented Nov 6, 2024 • edited Loading

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

Backporting merged PR #9508 into master

bdraco commented Nov 6, 2024

bdraco commented Nov 6, 2024

lenard-mosys commented Oct 18, 2024 •

edited

Loading

bdraco commented Oct 18, 2024 •

edited

Loading

codecov bot commented Oct 21, 2024 •

edited

Loading

codspeed-hq bot commented Oct 29, 2024 •

edited

Loading

patchback bot commented Nov 6, 2024 •

edited

Loading

patchback bot commented Nov 6, 2024 •

edited

Loading