Skip to content

Send an empty payload for NO_STATUS_RCVD #153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Conversation

mhils
Copy link
Member

@mhils mhils commented Dec 9, 2020

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 mhils force-pushed the close-without-status branch from e5a2b94 to e2acfc4 Compare December 9, 2020 13:20
Copy link
Member

@pgjones pgjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

I wonder if instead of this change the function should it raise a ValueError if the code passed in is NO_STATUS_RCVD on the basis that it can already send empty frames (code=None) and trying to explicitly send this code is an odd thing to do.

if code is None and reason is not None:
if code is CloseReason.NO_STATUS_RCVD:
code = None
if code is None and reason:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you change to reason is not None

Copy link
Member Author

@mhils mhils Dec 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wsproto currently generates CloseConnection events with an empty string as the payload.

reason = data[2:].decode("utf-8")

Would you prefer if we change that to a None payload?

@mhils
Copy link
Member Author

mhils commented Dec 9, 2020

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:

code: int
reason: Optional[str] = None

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
Copy link
Member

pgjones commented Aug 23, 2022

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 pgjones closed this Aug 23, 2022
@mhils
Copy link
Member Author

mhils commented Aug 23, 2022

Awesome, thanks for getting back at this!

@mhils mhils deleted the close-without-status branch August 23, 2022 14:49
pgjones added a commit that referenced this pull request Aug 23, 2022
This was originally per #153 but I felt it didn't make sense to
change. However, it does and without this change the autobahn tests
fail.
@pgjones
Copy link
Member

pgjones commented Aug 23, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants