Updated websocket consumers for ASGI v2.3. #2002
Conversation
carltongibson
changed the title
There was a problem hiding this comment.
Hi @kristjanvalur — thanks for this. Looks right.
Can you make the tests pass? (Missing a django_db marker from the looks of it.)
Refs https://asgi.readthedocs.io/en/latest/specs/www.html#websocket
|
I've updated, could you re-run the workflow? |
|
Oops, sorry about that, everything should now pass, including the linting. |
|
Would be great to have this merged at some point. |
|
I did the fixes with github.dev, could not test them, but I´m crossing my fingers. |
| """ | ||
| Closes the WebSocket from the server end | ||
| """ | ||
| message = {"type": "websocket.close"} | ||
| if code is not None and code is not True: |
There was a problem hiding this comment.
For my curiosity, why is there a check for the "code not being True"?
There was a problem hiding this comment.
Oh, hard questions! I think this is a compatibility check. It's been a while, but IIRC, there was a case where a "True" value was being passed to close... I need to look.
There was a problem hiding this comment.
Oh, this is not my code. Phew. So, your guess is as good as mine. Maybe in the distant past, someone would call close(True) for whatever reason and this is trying to allow for that? Nothing in the library or unit tests which is doing that, though.
| if reason: | ||
| message["reason"] = reason |
There was a problem hiding this comment.
I'm not sure if those conditionals are needed, but should be fine.
There was a problem hiding this comment.
The idea is to not pollute the response with an empty "reason" which is optional in any case.
This PR adds the "headers" field for
websocket.acceptmessages and the "reason" field forwebsocket.close, whichwere added in ASGI spec versions 2.1 and 2.3 respectively.