Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebSocketResponse.close() should wait for a close frame in its timeout window #9506

Open
1 task done
lenard-mosys opened this issue Oct 18, 2024 · 1 comment
Open
1 task done
Labels

Comments

@lenard-mosys
Copy link
Contributor

Describe the bug

After calling WebSocketResponse.close() the server waits for a single message from the client, and if it's not a close message, then it indicates an abnormal closure:

aiohttp/aiohttp/web_ws.py

Lines 489 to 506 in 13dc020

try:
async with async_timeout.timeout(self._timeout):
msg = await reader.read()
except asyncio.CancelledError:
self._set_code_close_transport(WSCloseCode.ABNORMAL_CLOSURE)
raise
except Exception as exc:
self._exception = exc
self._set_code_close_transport(WSCloseCode.ABNORMAL_CLOSURE)
return True
if msg.type is WSMsgType.CLOSE:
self._set_code_close_transport(msg.data)
return True
self._set_code_close_transport(WSCloseCode.ABNORMAL_CLOSURE)
self._exception = asyncio.TimeoutError()
return True

The problem with this is that a non-close client message might be in flight when the server sends its close message, and this results in an abnormal closure at the server's side, even if the client responds to the close message immediately. Here is the situation I'm describing:

aiohttp_ws_close_issue_opt

To my best understanding this is reported as an abnormal closure on the server side based on the implementation of WebSocketResponse.close().

To Reproduce

I didn't write a reproducer yet.

Expected behavior

During the timeout wait for a close message, and discard messages that arrive before that. Only report an abnormal closure if that close message never arrives.

Logs/tracebacks

NA

Python Version

all

aiohttp Version

master, as of writing 13dc0200d855bdfec1d4c4f3e1c9a1c66f88f1eb

multidict Version

NA

propcache Version

NA

yarl Version

NA

OS

NA

Related component

Server

Additional context

No response

Code of Conduct

  • I agree to follow the aio-libs Code of Conduct
@Dreamsorcerer
Copy link
Member

Yeah' that doesn't sound right. Feel free to make a PR, or maybe @bdraco might be interested in this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants