Fix 1230; don't call ensure_open in WebSocketProtocol.asgi_receive

[kylebebak]

Closes #1230. Reverts most of 9a3040c

We can't call WebSocketCommonProtocol.ensure_open before calling WebSocketCommonProtocol.recv, because there may be unread frames in the server's read queue that were sent by the client before the client sent the close frame. See this comment from the websockets source code

WebSocketCommonProtocol.ensure_open raises an exception and prevents the server from reading these unread frames. Neither the client, nor application code using uvicorn, has any way of knowing that frames successfully sent by the client to the server's read queue, before any close frame was sent, will never be read by the server.

The spec is clear on this: https://datatracker.ietf.org/doc/html/rfc6455#section-1.4

After sending a control frame indicating the connection should be closed, a peer does not send any further data; after receiving a control frame indicating the connection should be closed, a peer discards any further data received.

The current behavior in uvicorn is rather "a peer discards any further data received, and any data previously received"

Also worth noting, python-websockets/websockets#670 was eventually closed without any changes being made; in other words, there was no bug in websockets, and the maintainer agrees that calling await self.ensure_open() in recv is incorrect

---

I tested uvicorn after making these changes, and the behavior is as expected; the server can read frames in the read queue after a close frame is sent by the client, as long as those frames were sent before the close frame

[euri10]

ok, cant say I'm following everything here as it's been quite a while,
question is does this PR keeps intact the original intended fix for #244 ?

[euri10]

second thing, this may be a good idea to come up with tests for this

[kylebebak]

Hey @euri10

Indeed, this PR keeps the original fix to #244 , written by @adamhooper and outlined in this PR: #704

Small changes were made Adam Hooper's code because the websockets library has changed since then; WebSocketServerProtocol.close_code is now a property, not an attribute, so we can't assign to it.

The TypeError bug that @florimondmanca was able to reproduce, described here, can't be reproduced with this code. I added a test to ensure this bug doesn't reappear.

I also added a test that ensures the bug I found in #1230 doesn't come back. It's worth noting that this test immediately passed for the WSProtocol class without making any changes. It's good to know that WSProtocol doesn't have the bug in WebSocketProtocol.

[kylebebak]

Hey @euri10 @Kludex

I think this is ready to go, but please let me know if you think there's anything else we should add to it.

[kylebebak]

Hey @euri10

I just rolled back the code in websockets_impl.py on my machine to how it was in commit 99acac5, from 2 months ago, added a print to test_server_reject_connection, and ran it again.

In test_server_reject_connection, I observed the same behavior you just did, a close code of None for WSProtocol and 1006 for WebsocketProtocol.

In other words, this is preexisting behavior, and is not related to calling ensure_open in WebSocketProtocol.asgi_receive.

I don't know the websocket spec well enough to know whether the correct close code in this situation is None or 1006, or whether the spec even cares, but in any case we shouldn't gate the fix in this PR on unrelated, preexisting behavior which may or may not be an issue.

Here's what the tests printed out:

tests/protocols/test_websocket.py::test_server_reject_connection[H11Protocol-WSProtocol] INFO:     Started server process [15640]
INFO:     Uvicorn running on http://127.0.0.1:8000 (Press CTRL+C to quit)
INFO:     ('127.0.0.1', 52429) - "WebSocket /" 403
{'type': 'websocket.disconnect', 'code': None}
INFO:     Shutting down
PASSED
tests/protocols/test_websocket.py::test_server_reject_connection[H11Protocol-WebSocketProtocol] INFO:     Started server process [15640]
INFO:     Uvicorn running on http://127.0.0.1:8000 (Press CTRL+C to quit)
INFO:     ('127.0.0.1', 52430) - "WebSocket /" 403
INFO:     connection failed (403 Forbidden)
{'type': 'websocket.disconnect', 'code': 1006}
INFO:     connection closed
INFO:     Shutting down
PASSED
tests/protocols/test_websocket.py::test_server_reject_connection[HttpToolsProtocol-WSProtocol] INFO:     Started server process [15640]
INFO:     Uvicorn running on http://127.0.0.1:8000 (Press CTRL+C to quit)
INFO:     ('127.0.0.1', 52431) - "WebSocket /" 403
{'type': 'websocket.disconnect', 'code': None}
INFO:     Shutting down
PASSED
tests/protocols/test_websocket.py::test_server_reject_connection[HttpToolsProtocol-WebSocketProtocol] INFO:     Started server process [15640]
INFO:     Uvicorn running on http://127.0.0.1:8000 (Press CTRL+C to quit)
INFO:     ('127.0.0.1', 52432) - "WebSocket /" 403
INFO:     connection failed (403 Forbidden)
{'type': 'websocket.disconnect', 'code': 1006}

[euri10]

ok let's roll with this, thanks for the digging into finding the root cause of this @kylebebak
let's keep in mind that websocket close code discrepancy, not sure I wont forget since I'm not using that part of the library but never knows !

[kylebebak]

Thank you @euri10 for all the feedback and helping to get this merged!

I agree that it could be good to open an issue pointing out the discrepancy between the close codes when the server rejects the connection using WSProtocol vs WebsocketProtocol

I think this issue here has all the context we need. If you want to open an issue, we can just point to this PR

[kylebebak]

@euri10 @Kludex

One last thing... I think it would be good to release 0.15.1, so libraries that depend on uvicorn, e.g. FastAPI, can take advantage of this fix

The latest release of FastAPI depends on uvicorn[standard] (>=0.12.0,<0.16.0), so it would be able to pull in a fix from 0.15.1 immediately

[Kludex]

@euri10 I don't see any reason to bump a minor considering what we merged, do you?

We can close #1201, and aim for the patch if you think it's a patch. If it's a minor, I think we can continue on that PR.

But... FYK, FastAPI doesn't depend on uvicorn. fastapi[all] extra requirements do.

[euri10]

in all honesty I dont know, 0.15.0 was mid august iirc and I've been afk most of the time since so really cant say without looking more closely at what has been pushed

[Kludex]

I just confirmed, it's a minor. We bumped websockets and httptools, and there are also other commits that makes the next release a minor.

I'm going to reevaluate my wish list for 0.16.0 tonight on #1201 . If you have a PR that you wish as well @euri10 , feel free to add there.

[kylebebak]

Thanks for clarifying guys!