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

Add optional reason message and code to WebSocket.close #1107

Merged
merged 2 commits into from May 16, 2015

Conversation

Projects
None yet
2 participants
@Yoplitein
Contributor

Yoplitein commented May 15, 2015

Not sure why the payload length is limited to 125 bytes. Regular messages can be longer, so it doesn't seem to be an issue with vibe code. I found no mention of such a limit for close frames in the RFC.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig May 15, 2015

Member

According to the RFC, the status code should match the codes in section 7.4. For that reason and to avoid a dynamic memory allocation in the default case, I'd wrap the new code in an if (code != 0). I'd also change the order of parameters, because the code must always be set properly if a message is given, but not vice-versa. Otherwise looks good.

Member

s-ludwig commented May 15, 2015

According to the RFC, the status code should match the codes in section 7.4. For that reason and to avoid a dynamic memory allocation in the default case, I'd wrap the new code in an if (code != 0). I'd also change the order of parameters, because the code must always be set properly if a message is given, but not vice-versa. Otherwise looks good.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig May 15, 2015

Member

The 125 byte limit is mentioned in Section 5.5:

All control frames MUST have a payload length of 125 bytes or less
and MUST NOT be fragmented.

Member

s-ludwig commented May 15, 2015

The 125 byte limit is mentioned in Section 5.5:

All control frames MUST have a payload length of 125 bytes or less
and MUST NOT be fragmented.

@Yoplitein

This comment has been minimized.

Show comment
Hide comment
@Yoplitein

Yoplitein May 15, 2015

Contributor

The 125 byte limit is mentioned in Section 5.5:

Ah. I guess I didn't look well enough.

I'd wrap the new code in an if (code != 0). I'd also change the order of parameters

Done and done. Also rebased, so the builds should pass now.

Contributor

Yoplitein commented May 15, 2015

The 125 byte limit is mentioned in Section 5.5:

Ah. I guess I didn't look well enough.

I'd wrap the new code in an if (code != 0). I'd also change the order of parameters

Done and done. Also rebased, so the builds should pass now.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig May 16, 2015

Member

Thanks!

Member

s-ludwig commented May 16, 2015

Thanks!

s-ludwig added a commit that referenced this pull request May 16, 2015

Merge pull request #1107 from Yoplitein/websocketCloseReason
Add optional reason message and code to WebSocket.close

@s-ludwig s-ludwig merged commit 058c45d into vibe-d:master May 16, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment