Added spec defined websocket close enum #1990
Conversation
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.
This also fills in the close reason to match the close code in the case that it hasn't been provided.
|
@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. |
http/vibe/http/websockets.d
Outdated
| @@ -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; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Please let me know how you want to go with this since everything else is green on the PR.
There was a problem hiding this comment.
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).
http/vibe/http/websockets.d
Outdated
| version(assert) | ||
| assert(reason.length <= 123); | ||
| else | ||
| reason = reason[0 .. 123]; |
There was a problem hiding this comment.
Should be reason = reason[0 .. min($, 123)]; to avoid a range error.
There was a problem hiding this comment.
Thanks for this catch!
http/vibe/http/websockets.d
Outdated
| if(code == 0) | ||
| code = WebSocketCloseReason.normalClosure; | ||
|
|
||
| if(reason is null || reason.length == 0) |
There was a problem hiding this comment.
Nitpick: the reason is null is redundant here, if (reason.length == 0) would be sufficient.
This is done to avoid confusion in the API. Ideally, I could attach `toString` directly to the `WebSocketCloseReason`.
|
Unless you have any further change requests, I think I'm done with this PR. |
|
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. |
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.