Add WebSocketsSansIOProtocol
#2540
Conversation
|
@aaugustin Do you have availability to review this in the next days? |
|
Seems like I need to drop Python 3.8 to make my life easier. 👀 |
|
I'm making a note to look into it tomorrow. If I don't find time tomorrow, then I'll do over the Christmas break. |
| if event.opcode == Opcode.CONT: | ||
| self.handle_cont(event) |
There was a problem hiding this comment.
Recommendation on how to test this? The other implementations don't have the continuation frame 🤔
There was a problem hiding this comment.
This happens when a message is fragmented. See https://datatracker.ietf.org/doc/html/rfc6455#section-5.4 for context. In short:
- the first frame is TEXT or BINARY
- every subsequent frame is CONT
- the last frame has the FIN bit set
This is correctly implemented in handle_cont. As far as I can tell, ASGI doesn't support fragmented WebSocket messages so you have to buffer and reassemble.
For comparison, websockets provides both options — buffer and reassemble the message (if you don't care) or receive it frame by frame (if you need streaming). If you're curious, it's implemented here.
There was a problem hiding this comment.
I'm not very familiar with ASGI & uvicorn so I focused my review on the WebSocket protocol.
Your usage of websockets is straightforward.
I noticed that:
- You aren't detecting the case when
data_to_send()returns the empty bytestring to tell you to half-close the connection; that could be OK if you're confident that you're closing the transport in every scenario where the connection is terminating. - You aren't using the
close_expectedAPI to check whether you should close the TCP connection; that may be harmless on the server side because the server is expected to initiate the TCP closing handshake.
| @@ -104,7 +106,7 @@ async def websocket_app(scope: Scope, receive: ASGIReceiveCallable, send: ASGISe | |||
| elif message["type"] == "websocket.disconnect": | |||
| break | |||
|
|
|||
| async def open_connection(url): | |||
| async def open_connection(url: str): | |||
| async with websockets.client.connect(url) as websocket: | |||
| return websocket.open | |||
There was a problem hiding this comment.
return websocket.state is websockets.protocol.State.OPEN
Cf. https://websockets.readthedocs.io/en/stable/howto/upgrade.html#open-and-closed
| if event.opcode == Opcode.CONT: | ||
| self.handle_cont(event) |
There was a problem hiding this comment.
This happens when a message is fragmented. See https://datatracker.ietf.org/doc/html/rfc6455#section-5.4 for context. In short:
- the first frame is TEXT or BINARY
- every subsequent frame is CONT
- the last frame has the FIN bit set
This is correctly implemented in handle_cont. As far as I can tell, ASGI doesn't support fragmented WebSocket messages so you have to buffer and reassemble.
For comparison, websockets provides both options — buffer and reassemble the message (if you don't care) or receive it frame by frame (if you need streaming). If you're curious, it's implemented here.
| from typing import Any, Literal, cast | ||
| from urllib.parse import unquote | ||
|
|
||
| from websockets import InvalidState |
There was a problem hiding this comment.
from websockets.exceptions import InvalidState
There was a problem hiding this comment.
Rationale: I recommend that you avoid shurtcut imports going forwards.
|
|
||
| extensions = [] | ||
| if self.config.ws_per_message_deflate: | ||
| extensions = [ServerPerMessageDeflateFactory()] |
There was a problem hiding this comment.
Default settings lead to high-memory usage with marginal performance benefits e.g. 316KiB RAM per connection instead of 64KiB for an additional 10% compression in a basic benchmark.
FYI websockets defaults to:
ServerPerMessageDeflateFactory(
server_max_window_bits=12,
client_max_window_bits=12,
compress_settings={"memLevel": 5},
)See here for much more than you ever wanted to know.
| extensions=extensions, | ||
| max_size=self.config.ws_max_size, | ||
| logger=logging.getLogger("uvicorn.error"), | ||
| ) |
There was a problem hiding this comment.
Are you checking the Origin header elsewhere in uvicorn? If not, supporting it in the config would be good, as it helps defend against Cross-Site WebSocket Hijacking attacks. (I have forgotten about the details of this attack.)
|
|
||
| def connection_lost(self, exc: Exception | None) -> None: | ||
| code = 1005 if self.handshake_complete else 1006 | ||
| self.queue.put_nowait({"type": "websocket.disconnect", "code": code}) |
There was a problem hiding this comment.
In a normal closing handshake scenario where you generate a "websocket.disconnect" event when you receive a close frame, it looks like this will generete a second "websocket.disconnect" event when the TCP connection terminates, which sounds incorrect. Ideally, you'd generate that event only if you haven't generated one already.
| self.transport.close() | ||
|
|
||
| def eof_received(self) -> None: | ||
| pass |
There was a problem hiding this comment.
I would expect self.conn.receive_eof() here.
| data_type = self.curr_msg_data_type | ||
| msg: WebSocketReceiveEvent | ||
| if data_type == "text": | ||
| msg = {"type": "websocket.receive", data_type: self.bytes.decode()} |
There was a problem hiding this comment.
You may want to handle the case when decode() raises an exception. In that case, websockets closes the connection with code 1007 (invalid data).
Invalid UTF-8 is relatively uncommon so you could choose to ignore it. I forgot this case in the new asyncio implementation and it took some time to detect the bug.
|
|
||
| def handle_ping(self, event: Frame) -> None: | ||
| output = self.conn.data_to_send() | ||
| self.transport.write(b"".join(output)) |
There was a problem hiding this comment.
I would suggest that you always do this in handle_events() rather than only in handle_xxx() methods where you believe that websockets sent an automatic response. I'm not sure how much of a different it will make in practice but it feels cleaner.
No description provided.