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

Use correct WebSocket error codes #1753

Merged
merged 4 commits into from Nov 20, 2022
Merged

Use correct WebSocket error codes #1753

merged 4 commits into from Nov 20, 2022

Conversation

Kludex
Copy link
Sponsor Member

@Kludex Kludex commented Nov 1, 2022

This is a very confusing subject.

There are 4 ways for us to send websocket.disconnect to the application:

WSProtocol WebSocketProtocol Event Code
Server shutdown 1012 (Service Restart)
Client sent close frame Send the code passed to the server i.e. wsproto sends event.code, and websockets sends exc.code
Handshake failed 1006 (Abnormal Closure)
Connection lost without close frame 1005 (No Status Rcvd)

Note: If the connection was lost before the handshake was completed, then it should be a 1006.

This PR aims to match the behavior on both websockets implementations, and fixes https://github.com/encode/uvicorn/pull/1737/files#r1009855193.

@Kludex Kludex marked this pull request as draft November 1, 2022 07:49
@iudeen
Copy link
Contributor

iudeen commented Nov 1, 2022

1012 means server restarted and indicates client that it can retry connection right? What if the server is gone away completely? Shouldn't it be 1001?🤔

@Kludex Kludex marked this pull request as ready for review November 1, 2022 11:56
@@ -553,22 +553,22 @@ async def app(scope, receive, send):
@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):
async def test_connection_lost_before_handshake_complete(
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A more meaningful name on what scenario we are testing.

@@ -577,6 +577,8 @@ async def websocket_session(uri):
task.cancel()
send_accept_task.set()

assert disconnect_message == {"type": "websocket.disconnect", "code": 1006}
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The connection was closed without a close frame, so we send a 1006.


assert frames == [b"abc", b"abc", b"abc"]
assert disconnect_message == {"type": "websocket.disconnect", "code": 1000}
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The client closed the connection with a close frame, and the code interpreted by the WebSocket packages is 1000 i.e. normal closure.

async with websockets.client.connect(uri):
while True:
await asyncio.sleep(0.1)
await websockets.client.connect(uri)
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was changed because connect hangs until we are able to connect with the server. There was a previous misconception that we'd actually hit the while True logic.

We are never able to complete the handshake, as we don't send the websocket.accept, so we cancel the task below and let the application continue.

self.lost_connection_before_handshake = (
not self.handshake_completed_event.is_set()
)
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to know if the connection was lost before the handshake was completed to determine what code we'll be sending to the application.

  • If we lost connection before the handshake is completed, then we should send a 1006.
  • If we lost connection after the handshake is completed, then we should send a 1005.

@@ -335,11 +339,13 @@ async def asgi_receive(

await self.handshake_completed_event.wait()

if self.closed_event.is_set():
# If client disconnected, use WebSocketServerProtocol.close_code property.
if self.lost_connection_before_handshake:
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of lost_connection_before_handshake, I can create an attribute called lost_connection, and do the conditional before the await self.handshake_completed_event.wait() above. What do you prefer? 🤔

(I think this alternative will work, I'm not sure if I'm missing something...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meaning we wouldn’t hit await self.handshake_completed_event.wait() at all in connection is lost?

If that’s the case, it seems better 🤔

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I said doesn't work.

@Kludex Kludex mentioned this pull request Nov 1, 2022
13 tasks
@Kludex
Copy link
Sponsor Member Author

Kludex commented Nov 1, 2022

1012 means server restarted and indicates client that it can retry connection right? What if the server is gone away completely? Shouldn't it be 1001?thinking

Conceptually, if we think about a web server, I think it doesn't make sense to send a code that says that the server has stopped for good. We think about it as a long-living software.

In other words, when we call "shutdown", it's because it's going to be up again.

If the application wants, it can send a websocket.close with 1001, and shutdown the server. 🤷‍♂️

@iudeen
Copy link
Contributor

iudeen commented Nov 1, 2022

In other words, when we call "shutdown", it's because it's going to be up again.

Thanks! This justifies!

@Kludex
Copy link
Sponsor Member Author

Kludex commented Nov 17, 2022

Maybe this is not trivial, but I really want to include this in the release. Mainly because this is the "WebSockets release". 👀

iudeen
iudeen approved these changes Nov 17, 2022
Copy link
Contributor

@iudeen iudeen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good!😄

It'd be great if someone can confirm the comment on lost_connection_before_handshake change where Kludex asked about an alternative approach 😊

self.connections.remove(self)

if self.logger.level <= TRACE_LOG_LEVEL:
prefix = "%s:%d - " % tuple(self.client) if self.client else ""
self.logger.log(TRACE_LOG_LEVEL, "%sWebSocket connection lost", prefix)

self.handshake_complete = True
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is matching the behavior with the websockets implementation.

@Kludex Kludex merged commit 41156aa into master Nov 20, 2022
15 checks passed
@Kludex Kludex deleted the map-ws-error-codes branch November 20, 2022 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants