Check if handshake is completed before sending frame on wsproto shutdown #1737
Conversation
| if self.handshake_complete: | ||
| self.queue.put_nowait({"type": "websocket.disconnect", "code": 1012}) | ||
| output = self.conn.send(wsproto.events.CloseConnection(code=1012)) | ||
| self.transport.write(output) |
There was a problem hiding this comment.
This is the core here.
We want to check if the handshake was completed, because CloseConnection is not a valid event for wsproto.WSConnection.send() when the handshake is not completed.
This is the error these lines solve:
wsproto.utilities.LocalProtocolError: Event CloseConnection(code=1012, reason=None) cannot be sent during the handshakeMore about it on #596.
| else: | ||
| self.send_500_response() |
There was a problem hiding this comment.
The thing is... If the above is False, we still need to send something to the client, otherwise we are being rude. 😞
Without these lines, the client will have a:
curl: (52) Empty reply from serverWith these lines:
HTTP/1.1 500
content-length: 0
The curl command used was:
curl --include \
--no-buffer \
--header "Connection: Upgrade" \
--header "Upgrade: websocket" \
--header "Host: example.com:80" \
--header "Origin: http://example.com:80" \
--header "Sec-WebSocket-Key: SGVsbG8sIHdvcmxkIQ==" \
--header "Sec-WebSocket-Version: 13" \
http://localhost:8000/There was a problem hiding this comment.
Much neater. 😌
I did spend some time trying to figure out if we should we also include a textual description here, but I think it's probably okay as it currently stands.
| except BaseException: | ||
| self.logger.exception("Exception in ASGI application\n") |
There was a problem hiding this comment.
These are analogous. Just making it more readable.
| output = self.conn.send( | ||
| wsproto.events.AcceptConnection( | ||
| subprotocol=subprotocol, | ||
| extensions=extensions, | ||
| extra_headers=extra_headers, | ||
| if not self.transport.is_closing(): | ||
| output = self.conn.send( | ||
| wsproto.events.AcceptConnection( | ||
| subprotocol=subprotocol, | ||
| extensions=extensions, | ||
| extra_headers=extra_headers, | ||
| ) | ||
| ) | ||
| ) | ||
| self.transport.write(output) | ||
| self.transport.write(output) |
There was a problem hiding this comment.
This is necessary on this scenario, because the transport was closed on the shutdown(), it can also be that the client disconnected before accepting the connection...
| if self.ws_server.closing: | ||
| return {"type": "websocket.disconnect", "code": 1012} |
There was a problem hiding this comment.
This is to match the behavior on both implementations.
If we close the connection on shutdown, we should be sending 1012.
| @@ -82,7 +82,7 @@ plugins = | |||
|
|
|||
| [coverage:report] | |||
| precision = 2 | |||
| fail_under = 97.82 | |||
| fail_under = 97.92 | |||
|
Thanks @tomchristie 🙏 |
| @pytest.mark.anyio | ||
| @pytest.mark.parametrize("ws_protocol_cls", WS_PROTOCOLS) | ||
| @pytest.mark.parametrize("http_protocol_cls", HTTP_PROTOCOLS) | ||
| async def test_not_accept_on_connection_lost(ws_protocol_cls, http_protocol_cls): | ||
| send_accept_task = asyncio.Event() | ||
|
|
||
| async def app(scope, receive, send): | ||
| while True: | ||
| message = await receive() | ||
| if message["type"] == "websocket.connect": | ||
| await send_accept_task.wait() | ||
| await send({"type": "websocket.accept"}) | ||
| elif message["type"] == "websocket.disconnect": | ||
| break | ||
|
|
||
| async def websocket_session(uri): | ||
| async with websockets.client.connect(uri): | ||
| while True: | ||
| await asyncio.sleep(0.1) | ||
|
|
||
| config = Config(app=app, ws=ws_protocol_cls, http=http_protocol_cls, lifespan="off") | ||
| async with run_server(config): | ||
| task = asyncio.create_task(websocket_session("ws://127.0.0.1:8000")) | ||
| await asyncio.sleep(0.1) | ||
| task.cancel() | ||
| send_accept_task.set() |
There was a problem hiding this comment.
This test actually fails on the websockets implementation.
This fails before and after my changes, so it is actually not this PR who broke it.
There's a single small change on the websockets implementation on this PR, which actually handles the next test.
shutdownin wsproto impl #596I'm extremely happy about this PR. I understand WebSocket things now. 😅
How to test it?
Application:
Client (I've used curl):
curl --include \ --no-buffer \ --header "Connection: Upgrade" \ --header "Upgrade: websocket" \ --header "Host: example.com:80" \ --header "Origin: http://example.com:80" \ --header "Sec-WebSocket-Key: SGVsbG8sIHdvcmxkIQ==" \ --header "Sec-WebSocket-Version: 13" \ http://localhost:8000/Checklist
websocketsimplementation.