django / asgiref

ASGI specification and utilities
https://asgi.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
1.47k stars 209 forks source link

Spec question: `websocket.disconnect` doesn't support the `reason` field #459

Closed frankie567 closed 4 months ago

frankie567 commented 6 months ago

From the WebSocket specification, it's possible to set a "reason" when either the server or the client closes the connection: https://datatracker.ietf.org/doc/html/rfc6455#section-7.1.6

In the ASGI specification, this is well supported for the "Close - send event" (i.e. close sent by the server):

https://github.com/django/asgiref/blob/db3ff43e9fa1daf7c6b1cb67db8eec1885be911d/specs/www.rst?plain=1#L476-L496

However, for the "Disconnect - receive event (i.e. close sent by the client), it is not supported; only the code can be set:

https://github.com/django/asgiref/blob/db3ff43e9fa1daf7c6b1cb67db8eec1885be911d/specs/www.rst?plain=1#L440-L458

Is there a particular reason to omit this field for this particular event?

andrewgodwin commented 6 months ago

I do not remember a specific reason; it was likely either an oversight or the libraries I developed the spec against last decade didn't support it.

frankie567 commented 6 months ago

Thank you for your feedback 🙂 Then, do you see any inconvenience if I propose a PR to add this to the spec?

Kludex commented 6 months ago

Can you give me some days to reply here? I'm on vacation, but I'm back next week.

frankie567 commented 6 months ago

Sure! Enjoy your holidays 🌴

frankie567 commented 5 months ago

Hey @Kludex 👋 Did you have some time to think about this? 🙂

Kludex commented 5 months ago

It looks like an oversight, but... No one complained in years... Do you need it?

frankie567 commented 5 months ago

Well, not a really big issue but yes, I had one complain downstream in httpx-ws. Relevant context:

IMHO, it looks like a harmless addition that would make the spec a bit more compliant regarding WS spec. Happy to make the PR, if you agree 🙂

Kludex commented 5 months ago

Do the WebSockets clients send the "reason"?

frankie567 commented 5 months ago

They can, if I understand correctly the spec. It's apparently supported by browsers API: https://developer.mozilla.org/en-US/docs/Web/API/WebSocket/close

Kludex commented 5 months ago

Then I don't mind adding it.