websocketjs: Use Normal Closure (1000) close code in WebSocket.Close.

[johanbrandhorst]

When no status code is provided, Close will default to 1005, indicating that the websocket was closed with no status code. In the case of the Close function, the user has requested a termination of the websocket and the status code should indicate that.

[dmitshur]

Thanks for the pull request.

Would you mind telling how I can reproduce the before/after behaviors and test this PR?

[johanbrandhorst]

@shurcooL I discovered this when I was using the websocket bindings with the gorilla websocket implementation server side. The library returns a CloseError with a Code which can be used to test the behaviour. The old behaviour defaulted to 1005, and with the patch it returns 1000.

[dmitshur]

Sorry for the delay. I had to find a block of time to be able to review this.

I can reproduce this with github.com/gorilla/websocket as the WebSocket server:

echo: NextReader: websocket: close 1005 (no status)
echo: NextReader: websocket: close 1000 (normal)

But not with golang.org/x/net/websocket, because it doesn't expose the close frame's code.

The docs of https://developer.mozilla.org/en-US/docs/Web/API/WebSocket say:

close()

Closes the WebSocket connection or connection attempt, if any. If the connection is already CLOSED, this method does nothing.

void close(
  in optional unsigned short code,
  in optional DOMString reason
);

code (optional)

A numeric value indicating the status code explaining why the connection is being closed. If this parameter is not specified, a default value of 1000 (indicating a normal "transaction complete" closure) is assumed. See the list of status codes on the CloseEvent page for permitted values.

However, the status codes are documented at https://developer.mozilla.org/en-US/docs/Web/API/CloseEvent as:

Status code	Name	Description
1000	CLOSE_NORMAL	Normal closure; the connection successfully completed whatever purpose for which it was created.
1005	CLOSE_NO_STATUS	Reserved. Indicates that no status code was provided even though one was expected.

So getting 1005 when a 1000 code is not explicitly provided makes sense. I'm not sure what they mean by "If this parameter is not specified, a default value of 1000 (indicating a normal "transaction complete" closure) is assumed." Perhaps they meant that "1000 can be assumed", although the real status code is 1005?

But that might be a decision/bug/feature in gorilla/websocket...

In any case, I think explicitly providing 1000 close reason in this library's Close method makes sense, and I'm in favor of doing that.

I think it should also let us get rid of this case in the tests:

websocket/test/test/index.go

Line 62 in 15f7e1f

CloseNoReasonSpecified = 1005 // IE10 hates it when the server closes without sending a close reason

[dmitshur]

I think it should also let us get rid of this case in the tests:

websocket/test/test/index.go

Line 62 in 15f7e1f

CloseNoReasonSpecified = 1005 // IE10 hates it when the server closes without sending a close reason

Perhaps not. I misunderstood what it meant at first. The test uses golang.org/x/net/websocket as WebSocket server, which doesn't include a code in its close frame as I understand, so that case will need to stay for IE 10.

[dmitshur]

When no status code is provided, Close will default to 1005

I think it's more accurate to rephrase that as follows:

When no status code is provided in the close frame, the other side may interpret that as 1005 instead of 1000.

[dmitshur]

Perhaps not. I misunderstood what it meant at first. The test uses golang.org/x/net/websocket as WebSocket server, which doesn't include a code in its close frame as I understand, so that case will need to stay for IE 10.

Looking at the code more, it does use 1000 for its defaultCloseStatus. So not sure why IE 10 tests would've failed. But I'm not going to bother dealing with IE 10, especially given how hard it is to even run tests in the first place (#9).

[dmitshur]

I've amended your commit to make minor adjustments based on findings above. I've created a const for the close code. Also renamed the consts in the tests to match the exact names from the spec (see https://tools.ietf.org/html/rfc6455#section-11.7).

I've ran the tests, and they passed.

LGTM.

Thanks @johanbrandhorst!

@nightexcessive Does this look good to you?

[johanbrandhorst]

I appreciate that you're so thorough with this @shurcooL :). Thanks for the research!