[possible bug] 'code' is not labeled as optional

[caleb15]

In https://asgi.readthedocs.io/en/latest/specs/www.html#disconnect-receive-event-ws it does not say code is optional, yet it says "as per websocket spec", where code is optional ("may contain"). Later on in Close - send event it explicitly mentions that the code there is optional. In the first link should code be optional or required?

If code is optional I would be happy to submit a PR to this repo to update the docs.

This is not just a theoretical question, as we are facing a bug that may be related to this. In https://github.com/django/channels/blob/38324816c115567e198a573bc294d8345df5e8a9/channels/generic/websocket.py#L105 channels assumes that the code key exists, but in our case it doesn't exist and a KeyError is raised.

[andrewgodwin]

I believe it was intentional that it was not optional - the WebSocket spec also says:

If this Close control frame contains no status code, The WebSocket Connection Close Code is considered to be 1005

Thus, I would say servers should default that value to 1005 if it is not contained in the frame

[caleb15]

So should channels default the value to 1005, then? Or maybe Uvicorn should specify code (link)? Or both?

[andrewgodwin]

If I was being strict, I'd say the servers should always populate this field (so uvicorn should probably add it), but in the interests of compatibility it makes sense for channels to default it as well. I'll update the spec to clarify this, if that makes sense?

[caleb15]

Yep, that makes sense. Thanks!

[andrewgodwin]

Alright, clarified in 5118b70 - thanks for highlighting this!

[Kludex]

So should channels default the value to 1005, then? Or maybe Uvicorn should specify code (link)? Or both?

Uvicorn team is already aware of this. 👀 There's a PR in WIP adding the code on those. 🙏

[andrewgodwin]

Great! Hopefully it matches the default 1005 value suggested here?

[Kludex]

Not really, and now that you've mentioned... We were actually using 1000 on some places as default, see:

https://github.com/encode/uvicorn/blob/6cf6abacd57a96c8f189fdb8d5544cd45a88f35a/uvicorn/protocols/websockets/wsproto_impl.py#L294-L304

[andrewgodwin]

Yeah, 1000 is the default for the app-initiated close, whereas 1005 is the default for the connection/client-initated close. It's... a confusing system.

[Kludex]

I see... So on the PR: https://github.com/encode/uvicorn/pull/1441/files

The first one should be 1005. Correct?

[Kludex]

Actually... Both on that PR should be 1005? 🤔

[andrewgodwin]

Yeah, both of those should be, since they are disconnect. disconnect = 1005, close = 1000 as defaults.

[Kludex]

Thanks Andrew :)

[carltongibson]

@caleb15

...in the interests of compatibility it makes sense for channels to default it as well

If you think there's a change in Channels to be made for this, please do propose it over there. (Thanks 😉)

[caleb15]

@carltongibson, sure. I figure I might as well make the proposal a PR because the change is so easy to make: django/channels#1905

[Kludex]

I've created a table on encode/uvicorn#1753 about the websocket codes we should be sending back, jfyk.