wsproto icon indicating copy to clipboard operation
wsproto copied to clipboard

Send an empty payload for NO_STATUS_RCVD

Open mhils opened this issue 3 years ago • 1 comments

x = wsproto.events.CloseConnection(wsproto.frame_protocol.CloseReason.NO_STATUS_RCVD)
ws.send(x)

currently emits a close frame with status code 1000 (NORMAL_CLOSURE) instead of not sending a payload. This PR fixes this. NO_STATUS_RCVD should probably be more generally called NO_STATUS_CODE, but that would unnecessarily break compatibility.

mhils avatar Dec 09 '20 13:12 mhils

If I understand correctly the 1005 (NO_STATUS_RCVD) should not be sent, but rather should be substituted if there is no status code (on received close frames).

Yes. The current behavior is that if one tries to send a 1005, it is silently substituted with a 1000. My proposal is to change that to not sending a status code instead. It would definitely be wrong to send an actual status code of 1005 on the wire.


The problem at its core is that wsproto currently uses the special value NO_STATUS_RCVD when receiving frames without status code, but asks for a different special value (code=None) to send them. That feels inconsistent and is annoying when proxying messages for example.

At the moment the CloseConnection also does not declare code as optional, so strictly speaking you can't use the high-level API to send empty close frames:

https://github.com/python-hyper/wsproto/blob/38716a9e14dd5ff4512426567d40a09912e060fb/src/wsproto/events.py#L181-L182

My proposal would be we consistently use one special value (None or NO_STATUS_RCVD), or at least accept 1005 for sending as well so that it doesn't need to be special-cased. I'd be fine with either case, but since the spec already proposes 1005 that seems like a reasonable choice. Does that make sense?

mhils avatar Dec 09 '20 14:12 mhils

I've merged manually with a slight change to the reason None checking - as reason=="" has no affect on the payload and I wanted to keep the diff to the code change only.

pgjones avatar Aug 23 '22 14:08 pgjones

Awesome, thanks for getting back at this!

mhils avatar Aug 23 '22 14:08 mhils

The reason change did in the end make sense (to keep the autobahn tests passing), have re-added in d970644642e2529f61c112d1ccbc095716e5e97c.

pgjones avatar Aug 23 '22 16:08 pgjones