[major] Change WebSocket#{p{i,o}ng,send}() behavior (#1532)

- If the `readyState` attribute is `CONNECTING`, throw an exception. - If the `readyState` attribute is `CLOSING` or `CLOSED` - Increase the `bufferedAmount` attribute by the length of the `data` argument in bytes. - If specified, call the `callback` function with an error. Fixes #1515
websockets · Apr 25, 2019 · 5d751fb · 5d751fb
1 parent 5479eae
commit 5d751fb
Show file tree

Hide file tree

Showing 3 changed files with 325 additions and 125 deletions.
diff --git a/README.md b/README.md
@@ -35,7 +35,6 @@ can use one of the many wrappers available on npm, like
   - [Server broadcast](#server-broadcast)
   - [echo.websocket.org demo](#echowebsocketorg-demo)
   - [Other examples](#other-examples)
-- [Error handling best practices](#error-handling-best-practices)
 - [FAQ](#faq)
   - [How to get the IP address of the client?](#how-to-get-the-ip-address-of-the-client)
   - [How to detect and close broken connections?](#how-to-detect-and-close-broken-connections)
@@ -309,30 +308,6 @@ examples folder.
 
 Otherwise, see the test cases.
 
-## Error handling best practices
-
-```js
-// If the WebSocket is closed before the following send is attempted
-ws.send('something');
-
-// Errors (both immediate and async write errors) can be detected in an optional
-// callback. The callback is also the only way of being notified that data has
-// actually been sent.
-ws.send('something', function ack(error) {
-  // If error is not defined, the send has been completed, otherwise the error
-  // object will indicate what failed.
-});
-
-// Immediate errors can also be handled with `try...catch`, but **note** that
-// since sends are inherently asynchronous, socket write failures will *not* be
-// captured when this technique is used.
-try {
-  ws.send('something');
-} catch (e) {
-  /* handle error */
-}
-```
-
 ## FAQ
 
 ### How to get the IP address of the client?

diff --git a/lib/websocket.js b/lib/websocket.js
@@ -21,6 +21,7 @@ const {
   kWebSocket,
   NOOP
 } = require('./constants');
+const { toBuffer } = require('./buffer-util');
 
 const readyStates = ['CONNECTING', 'OPEN', 'CLOSING', 'CLOSED'];
 const protocolVersions = [8, 13];
@@ -57,6 +58,7 @@ class WebSocket extends EventEmitter {
     this._socket = null;
 
     if (address !== null) {
+      this._bufferedAmount = 0;
       this._isServer = false;
       this._redirects = 0;
 
@@ -112,7 +114,7 @@ class WebSocket extends EventEmitter {
    * @type {Number}
    */
   get bufferedAmount() {
-    if (!this._socket) return 0;
+    if (!this._socket) return this._bufferedAmount;
 
     //
     // `socket.bufferSize` is `undefined` if the socket is closed.
@@ -252,6 +254,10 @@ class WebSocket extends EventEmitter {
    * @public
    */
   ping(data, mask, cb) {
+    if (this.readyState === WebSocket.CONNECTING) {
+      throw new Error('WebSocket is not open: readyState 0 (CONNECTING)');
+    }
+
     if (typeof data === 'function') {
       cb = data;
       data = mask = undefined;
@@ -260,17 +266,13 @@ class WebSocket extends EventEmitter {
       mask = undefined;
     }
 
-    if (this.readyState !== WebSocket.OPEN) {
-      const err = new Error(
-        `WebSocket is not open: readyState ${this.readyState} ` +
-          `(${readyStates[this.readyState]})`
-      );
+    if (typeof data === 'number') data = data.toString();
 
-      if (cb) return cb(err);
-      throw err;
+    if (this.readyState !== WebSocket.OPEN) {
+      sendAfterClose(this, data, cb);
+      return;
     }
 
-    if (typeof data === 'number') data = data.toString();
     if (mask === undefined) mask = !this._isServer;
     this._sender.ping(data || EMPTY_BUFFER, mask, cb);
   }
@@ -284,6 +286,10 @@ class WebSocket extends EventEmitter {
    * @public
    */
   pong(data, mask, cb) {
+    if (this.readyState === WebSocket.CONNECTING) {
+      throw new Error('WebSocket is not open: readyState 0 (CONNECTING)');
+    }
+
     if (typeof data === 'function') {
       cb = data;
       data = mask = undefined;
@@ -292,17 +298,13 @@ class WebSocket extends EventEmitter {
       mask = undefined;
     }
 
-    if (this.readyState !== WebSocket.OPEN) {
-      const err = new Error(
-        `WebSocket is not open: readyState ${this.readyState} ` +
-          `(${readyStates[this.readyState]})`
-      );
+    if (typeof data === 'number') data = data.toString();
 
-      if (cb) return cb(err);
-      throw err;
+    if (this.readyState !== WebSocket.OPEN) {
+      sendAfterClose(this, data, cb);
+      return;
     }
 
-    if (typeof data === 'number') data = data.toString();
     if (mask === undefined) mask = !this._isServer;
     this._sender.pong(data || EMPTY_BUFFER, mask, cb);
   }
@@ -312,31 +314,31 @@ class WebSocket extends EventEmitter {
    *
    * @param {*} data The message to send
    * @param {Object} options Options object
-   * @param {Boolean} options.compress Specifies whether or not to compress `data`
+   * @param {Boolean} options.compress Specifies whether or not to compress
+   *     `data`
    * @param {Boolean} options.binary Specifies whether `data` is binary or text
    * @param {Boolean} options.fin Specifies whether the fragment is the last one
    * @param {Boolean} options.mask Specifies whether or not to mask `data`
    * @param {Function} cb Callback which is executed when data is written out
    * @public
    */
   send(data, options, cb) {
+    if (this.readyState === WebSocket.CONNECTING) {
+      throw new Error('WebSocket is not open: readyState 0 (CONNECTING)');
+    }
+
     if (typeof options === 'function') {
       cb = options;
       options = {};
     }
 
-    if (this.readyState !== WebSocket.OPEN) {
-      const err = new Error(
-        `WebSocket is not open: readyState ${this.readyState} ` +
-          `(${readyStates[this.readyState]})`
-      );
+    if (typeof data === 'number') data = data.toString();
 
-      if (cb) return cb(err);
-      throw err;
+    if (this.readyState !== WebSocket.OPEN) {
+      sendAfterClose(this, data, cb);
+      return;
     }
 
-    if (typeof data === 'number') data = data.toString();
-
     const opts = Object.assign(
       {
         binary: typeof data !== 'string',
@@ -723,6 +725,38 @@ function abortHandshake(websocket, stream, message) {
   }
 }
 
+/**
+ * Handle cases where the `ping()`, `pong()`, or `send()` methods are called
+ * when the `readyState` attribute is `CLOSING` or `CLOSED`.
+ *
+ * @param {WebSocket} websocket The WebSocket instance
+ * @param {*} data The data to send
+ * @param {Function} cb Callback
+ * @private
+ */
+function sendAfterClose(websocket, data, cb) {
+  if (data) {
+    const length = toBuffer(data).length;
+
+    //
+    // The `_bufferedAmount` property is used only when the peer is a client and
+    // the opening handshake fails. Under these circumstances, in fact, the
+    // `setSocket()` method is not called, so the `_socket` and `_sender`
+    // properties are set to `null`.
+    //
+    if (websocket._socket) websocket._sender._bufferedBytes += length;
+    else websocket._bufferedAmount += length;
+  }
+
+  if (cb) {
+    const err = new Error(
+      `WebSocket is not open: readyState ${websocket.readyState} ` +
+        `(${readyStates[websocket.readyState]})`
+    );
+    cb(err);
+  }
+}
+
 /**
  * The listener of the `Receiver` `'conclude'` event.
  *