send on a closed WebSocket should not throw an exception

[jfirebaugh]

• I've searched for any related issues and avoided creating a duplicate
  issue.

Description

For compatibility with browser implementations, the HTML standard, and RFC 6455, calling send on a WebSocket whose readyState is CLOSING or CLOSED should discard the data and not throw an exception. (As a QOL issue, it may log an error to the console.)

Currently it throws an exception such as "WebSocket is not open: readyState 2 (CLOSING)".

Reproducible in:

• version: 5.2.2
• Node.js version(s): v10.14.1
• OS version(s): macOS 10.14.3

Steps to reproduce:

1. Start a WebSocket server on port 9000.

2. Run the following in node:

const WebSocket = require("ws");
const ws = new WebSocket("ws://localhost:9000");
ws.onopen = () => {
  ws.close();
  ws.send("test");
  console.log("sent");
};

2. Open the following in browsers:

<script>
const ws = new WebSocket("ws://localhost:9000");
ws.onopen = () => {
  ws.close();
  ws.send("test");
  console.log("sent");
};
</script>

Expected result:

All result in "sent" being printed to the console.

Actual result:

• Node prints Error: WebSocket is not open: readyState 2 (CLOSING), with an exception stack trace pointing to websocket.js:320.
• Chrome prints the error diagnostic "WebSocket is already in CLOSING or CLOSED state.", but does not throw an exception ("sent" is printed).
• Firefox prints "sent" and no error.
• Safari prints "sent" and no error.

References:

HTML Standard:

The send(data) method transmits data using the connection. If the readyState attribute is CONNECTING, it must throw an "InvalidStateError" DOMException. Otherwise, the user agent must run the appropriate set of steps from the following list:

Note that an exception must be thrown only in the CONNECTING state, and the "following list" does not include any steps that involve throwing an exception.

RFC 6455:

The endpoint MUST ensure the WebSocket connection is in the OPEN state (cf. Sections 4.1 and 4.2.2.) If at any point the state of the WebSocket connection changes, the endpoint MUST abort the following steps.

Note "abort the following steps"; i.e. stop processing, but do not raise an exception.

[lpinca]

This changed over the years but originally the browser implementations threw and I think (this pre-dates me) it is the reason why it was implemented in this way.

The spec is not clear in this regard and it is confirmed by the fact that different implementations have different behaviors.

I think what we have is actually somehow consistent with Node.js streams where an error is emitted if the stream is closed. We throw and do not emit, but only when not using a callback. If ws is wrapped in a Duplex stream it works as expected (the error is emitted).

I'm not sure if it makes sense to change this. It would be a breaking change for no clear benefit in my opinion. I'm also not sure what the correct behavior should be.

[jfirebaugh]

The spec is not clear in this regard and it is confirmed by the fact that different implementations have different behaviors.

I disagree; I think the spec is clear that send on a closed socket should not throw, and all browser implementations agree on that. The fact that Chrome prints a diagnostic error message does not constitute "different behavior" with regards to throwing an exception -- Chrome does not throw an exception, nor do Firefox or Safari.

In addition to the spec references I've quoted above, consider the description of socket.bufferedAmount:

Returns the number of bytes of application data (UTF-8 text and binary data) that have been queued using send() but not yet been transmitted to the network.

If the WebSocket connection is closed, this attribute's value will only increase with each call to the send() method. (The number does not reset to zero once the connection closes.)

If send on a closed socket was intended to throw, the latter paragraph would make no sense.

[lpinca]

I was talking about the RFC, not the WHATWG spec which is "new" and browser focused. What should be in your opinion the expected behavior? Buffer messages indefinitely with no feedback to the user other than an always increasing bufferedAmound?

[jfirebaugh]

Ah, I think I may have misunderstood the intent of this module -- I had thought that its WebSocket class was intended to be an implementation of the WHATWG spec specifically. It sounds like you're saying that it's a more general "JS WebSocket" implementation based on the RFC, and congruence with the browser API is coincidental. If that's the case, I apologize for the noise.

My use case is focused on testing the browser/client side of a WebSocket client/server application. I was using ws so that I could actually still run the client tests in node, but it sounds like I should instead be using the WebSocket implementation from jsdom, which wraps ws with a WHATWG-spec-compliant API.

[lpinca]

We strive to be browser compatible but there are some differences. Here is a non comprehensive list of differences:

• ws allows to send fragmented messages.
• ws allows to set the binaryType attribute to 'fragments' to get all fragments of a message instead of single message.
• In ws the default value for the binaryType attribute is the non standard 'nodebuffer' instead of 'blob'. 'blob' is not supported.
• ws allows to send ping/pong frames.
• ws allows to specify whether a message should be compressed or not before sending it as per permessage-deflate specification.
• ws allows the developer to know when a message/fragment is written out using the send callback.
• ws allows to send custom headers when the opening handshake is initiated.
• In ws the bufferedAmount attribute works differently (bufferedAmount property is broken #492).

I'm fine with changing send() behavior to only throw when the readyState is CONNECTING but I think that if a callback is provided, it should always be called with an error if readyState is not OPEN. Furthermore, in my opinion, if send() is called when readyState is CLOSING or CLOSED, the data should not be buffered as this will create a leak. In this case send() should only call the callback with an error, increase the bufferedAmount value, and discard the actual data.

[jfirebaugh]

Thanks for the details! Your proposal sounds good to me.

[TrevorSundberg]

I'm wondering is there any equivalent of a readystatechange or closing event? I'm fine with the exception as is because I think it's important to disconnect any code paths that could be calling send(), however even if you properly handle close you can still get an exception fired on trying to send when the state is CLOSING. Is the only way to handle this to check for CLOSING explicitly before calling send()?

[lpinca]

@TrevorSundberg see #964.

Is the only way to handle this to check for CLOSING explicitly before calling send()?

Yes use a callback or check for readyState === WebSocket.OPEN.