search

python-hyper / wsproto

Sans-IO WebSocket protocol implementation
https://wsproto.readthedocs.io/
MIT License
261 stars 38 forks source link

Send an empty payload for NO_STATUS_RCVD #153

Closed mhils closed 2 years ago

mhils commented 3 years ago 
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 commented 3 years ago

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?

pgjones commented 2 years ago

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.

mhils commented 2 years ago

Awesome, thanks for getting back at this!

pgjones commented 2 years ago

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