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

Added spec defined websocket close enum #1990

Merged
merged 8 commits into from Dec 11, 2017

Conversation

Projects
None yet
2 participants
@andrewbenton
Contributor

andrewbenton commented Dec 2, 2017

This enum and function provide access to the defined websocket close
codes and will also generate the reason string based off of the code if
one isn't already defined. This saves library users from having to look
up documentation.

andrewbenton added some commits Dec 2, 2017

Added spec defined websocket close enum
This enum and function provide access to the defined websocket close
codes and will also generate the reason string based off of the code if
one isn't already defined.  This saves library users from having to look
up documentation.
Make use of WebSocketCloseReason over magic nums
This also fills in the close reason to match the close code in the case
that it hasn't been provided.
@andrewbenton

This comment has been minimized.

Contributor

andrewbenton commented Dec 4, 2017

@s-ludwig Would you mind re-running that single failing test? I can't determine from looking at the logs why it might have failed.

@s-ludwig

Thanks! Looks good apart from the two mentioned issues. I've restarted the failing build, indeed that just looks like a temporary failure.

@@ -567,8 +680,18 @@ final class WebSocket {
*/
void close(short code = 0, scope const(char)[] reason = "")
{
//assume the default is normal, intentional closure
if(code == 0)
code = WebSocketCloseReason.normalClosure;

This comment has been minimized.

@s-ludwig

s-ludwig Dec 4, 2017

Member

This would remove the possibility to send a closing frame without a code/reason, which may be desirable for efficiency if a closing reason isn't used anyway. Do you think that sending no status is an issue for existing endpoints in practice? In that case, the other option would be to change the default parameter to code = WebSocketCloseReason.normalClosure and to define WebSocketCloseReason.none = 0 to allow explicitly skipping the status.

This comment has been minimized.

@andrewbenton

andrewbenton Dec 4, 2017

Contributor

I like the idea of adding a WebSocketCloseReason.none and defaulting to normal. I'm not sure if sending no status is an issue, though it probably acts as an unexpected closure. From an API standpoint, I had expected that the default was a normal closure.

This comment has been minimized.

@s-ludwig

s-ludwig Dec 4, 2017

Member

My expectation was that it is usually trated as a normal closure, except if the peer application actually expects a status code, but I would be fine with the .none solution.

BTW, a related question is if the reason string should be omitted if none is given explicitly. Sending a generic string doesn't add much value and may be unwanted use of bandwidth for applications where this is critical.

This comment has been minimized.

@andrewbenton

andrewbenton Dec 4, 2017

Contributor

With the generic string, are you referring to the 0 case, or the spec-recognized cases, or both? I admit that I like the idea of providing that string, though saving bandwidth is appealing too.

My motivations on this are driven by having to look up the close reason codes on the server when we receive a websocket close and also on the browser's side when we get a number only and I have to look it up.

Do you think that adding another method would be better, or maybe using the defaulting string behavior only for non-release versions?

This comment has been minimized.

@andrewbenton

andrewbenton Dec 5, 2017

Contributor

Please let me know how you want to go with this since everything else is green on the PR.

This comment has been minimized.

@s-ludwig

s-ludwig Dec 5, 2017

Member

As for the string, I think it would be fine to just treat null as a special case to still allow people to disable the reason phrase if bandwidth is critical. So the default phrase would just be set if (reason !is null && reason.length == 0). Using different behavior for debug/release is something I'd rather not do in this case, since I'd rate the reason phrase to be slightly more than just a debugging aid (as opposed to sending a stack trace or similar things).

version(assert)
assert(reason.length <= 123);
else
reason = reason[0 .. 123];

This comment has been minimized.

@s-ludwig

s-ludwig Dec 4, 2017

Member

Should be reason = reason[0 .. min($, 123)]; to avoid a range error.

This comment has been minimized.

@andrewbenton

andrewbenton Dec 4, 2017

Contributor

Thanks for this catch!

if(code == 0)
code = WebSocketCloseReason.normalClosure;
if(reason is null || reason.length == 0)

This comment has been minimized.

@s-ludwig

s-ludwig Dec 4, 2017

Member

Nitpick: the reason is null is redundant here, if (reason.length == 0) would be sufficient.

andrewbenton added some commits Dec 4, 2017

Rename module-level toString to closeReasonString
This is done to avoid confusion in the API.  Ideally, I could attach
`toString` directly to the `WebSocketCloseReason`.
@andrewbenton

This comment has been minimized.

Contributor

andrewbenton commented Dec 8, 2017

Unless you have any further change requests, I think I'm done with this PR.

@s-ludwig

This comment has been minimized.

Member

s-ludwig commented Dec 11, 2017

Merging now, thanks again! I was just waiting until the 0.8.2 release was out, so that I didn't have to create a release branch.

@s-ludwig s-ludwig merged commit 4284ca3 into vibe-d:master Dec 11, 2017

4 checks passed

codecov/patch 98.484% of diff hit (target 60.209%)
Details
codecov/project 60.886% (+0.676%) compared to e424042
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
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