Make the option work on the server, add JSDoc, and simplify tests

Author: lpinca

doc/ws.md

@@ -80,7 +80,8 @@ This class represents a WebSocket server. It extends the `EventEmitter`.
     in response to a ping. Defaults to `true`.
   - `backlog` {Number} The maximum length of the queue of pending connections.
   - `clientTracking` {Boolean} Specifies whether or not to track clients.
-  - `closeTimeout` {Number} Timeout in milliseconds for graceful close.
+  - `closeTimeout` {Number} Timeout in milliseconds for graceful close. Defaults
+    to 30000.
   - `handleProtocols` {Function} A function which can be used to handle the
     WebSocket subprotocols. See description below.
   - `host` {String} The hostname where to bind the server.
@@ -305,7 +306,8 @@ This class represents a WebSocket. It extends the `EventEmitter`.
     the WHATWG standardbut may negatively impact performance.
   - `autoPong` {Boolean} Specifies whether or not to automatically send a pong
     in response to a ping. Defaults to `true`.
-  - `closeTimeout` {Number} Timeout in milliseconds for graceful close.
+  - `closeTimeout` {Number} Timeout in milliseconds for graceful close. Defaults
+    to 30000.
   - `finishRequest` {Function} A function which can be used to customize the
     headers of each HTTP request before it is sent. See description below.
   - `followRedirects` {Boolean} Whether or not to follow redirects. Defaults to

lib/constants.js

@@ -7,6 +7,7 @@ if (hasBlob) BINARY_TYPES.push('blob');
 
 module.exports = {
   BINARY_TYPES,
+  CLOSE_TIMEOUT: 30000,
   EMPTY_BUFFER: Buffer.alloc(0),
   GUID: '258EAFA5-E914-47DA-95CA-C5AB0DC85B11',
   hasBlob,

lib/websocket-server.js

@@ -11,7 +11,7 @@ const extension = require('./extension');
 const PerMessageDeflate = require('./permessage-deflate');
 const subprotocol = require('./subprotocol');
 const WebSocket = require('./websocket');
-const { GUID, kWebSocket } = require('./constants');
+const { CLOSE_TIMEOUT, GUID, kWebSocket } = require('./constants');
 
 const keyRegex = /^[+/0-9A-Za-z]{22}==$/;
 
@@ -38,6 +38,8 @@ class WebSocketServer extends EventEmitter {
    *     pending connections
    * @param {Boolean} [options.clientTracking=true] Specifies whether or not to
    *     track clients
+   * @param {Number} [options.closeTimeout=30000] Timeout in milliseconds for
+   *     graceful close
    * @param {Function} [options.handleProtocols] A hook to handle protocols
    * @param {String} [options.host] The hostname where to bind the server
    * @param {Number} [options.maxPayload=104857600] The maximum allowed message
@@ -67,6 +69,7 @@ class WebSocketServer extends EventEmitter {
       perMessageDeflate: false,
       handleProtocols: null,
       clientTracking: true,
+      closeTimeout: CLOSE_TIMEOUT,
       verifyClient: null,
       noServer: false,
       backlog: null, // use default (511 as implemented in net.js)

lib/websocket.js

@@ -18,6 +18,7 @@ const { isBlob } = require('./validation');
 
 const {
   BINARY_TYPES,
+  CLOSE_TIMEOUT,
   EMPTY_BUFFER,
   GUID,
   kForOnEventAttribute,
@@ -32,7 +33,6 @@ const {
 const { format, parse } = require('./extension');
 const { toBuffer } = require('./buffer-util');
 
-const closeTimeout = 30 * 1000;
 const kAborted = Symbol('kAborted');
 const protocolVersions = [8, 13];
 const readyStates = ['CONNECTING', 'OPEN', 'CLOSING', 'CLOSED'];
@@ -88,6 +88,7 @@ class WebSocket extends EventEmitter {
       initAsClient(this, address, protocols, options);
     } else {
       this._autoPong = options.autoPong;
+      this._closeTimeout = options.closeTimeout;
       this._isServer = true;
     }
   }
@@ -629,6 +630,8 @@ module.exports = WebSocket;
  *     times in the same tick
  * @param {Boolean} [options.autoPong=true] Specifies whether or not to
  *     automatically send a pong in response to a ping
+ * @param {Number} [options.closeTimeout=30000] Timeout in milliseconds for
+ *     graceful close
  * @param {Function} [options.finishRequest] A function which can be used to
  *     customize the headers of each http request before it is sent
  * @param {Boolean} [options.followRedirects=false] Whether or not to follow
@@ -655,6 +658,7 @@ function initAsClient(websocket, address, protocols, options) {
   const opts = {
     allowSynchronousEvents: true,
     autoPong: true,
+    closeTimeout: CLOSE_TIMEOUT,
     protocolVersion: protocolVersions[1],
     maxPayload: 100 * 1024 * 1024,
     skipUTF8Validation: false,
@@ -1291,7 +1295,7 @@ function senderOnError(err) {
 function setCloseTimer(websocket) {
   websocket._closeTimer = setTimeout(
     websocket._socket.destroy.bind(websocket._socket),
-    websocket._closeTimeout || closeTimeout
+    websocket._closeTimeout
   );
 }
 

test/websocket-server.test.js

@@ -140,6 +140,22 @@ describe('WebSocketServer', () => {
           });
         });
       });
+
+      it('honors the `closeTimeout` option', (done) => {
+        const closeTimeout = 1000;
+        const wss = new WebSocket.Server({ closeTimeout, port: 0 }, () => {
+          const ws = new WebSocket(`ws://localhost:${wss.address().port}`);
+        });
+
+        wss.on('connection', (ws) => {
+          ws.on('close', () => {
+            wss.close(done);
+          });
+
+          ws.close();
+          assert.strictEqual(ws._closeTimer._idleTimeout, closeTimeout);
+        });
+      });
     });
 
     it('emits an error if http server bind fails', (done) => {

test/websocket.test.js

@@ -232,6 +232,24 @@ describe('WebSocket', () => {
           ws.ping();
         });
       });
+
+      it('honors the `closeTimeout` option', (done) => {
+        const wss = new WebSocket.Server({ port: 0 }, () => {
+          const closeTimeout = 1000;
+          const ws = new WebSocket(`ws://localhost:${wss.address().port}`, {
+            closeTimeout
+          });
+
+          ws.on('open', () => {
+            ws.close();
+            assert.strictEqual(ws._closeTimer._idleTimeout, closeTimeout);
+          });
+
+          ws.on('close', () => {
+            wss.close(done);
+          });
+        });
+      });
     });
   });
 
@@ -3454,41 +3472,6 @@ describe('WebSocket', () => {
         });
       });
     });
-
-    it('uses closeTimeout milliseconds for the close timer', (done) => {
-      const timeoutMs = 5 * 1000;
-
-      const wss = new WebSocket.Server({ port: 0, closeTimeout: timeoutMs}, () => {
-        const ws = new WebSocket(`ws://localhost:${wss.address().port}`, undefined, {closeTimeout: timeoutMs});
-
-        assert.strictEqual(ws._closeTimeout, timeoutMs);
-
-        ws.on('close', (code, reason) => {
-          assert.strictEqual(code, 1000);
-          assert.deepStrictEqual(reason, Buffer.from('some reason'));
-          wss.close(done);
-        });
-
-        ws.on('open', () => {
-          let callbackCalled = false;
-
-          assert.strictEqual(ws._closeTimer, null);
-
-          ws.send('foo', () => {
-            callbackCalled = true;
-          });
-
-          ws.close(1000, 'some reason');
-
-          //
-          // Check that the close timer is set even if the `Sender.close()`
-          // callback is not called.
-          //
-          assert.strictEqual(callbackCalled, false);
-          assert.strictEqual(ws._closeTimer._idleTimeout, timeoutMs);
-        });
-      });
-    });
   });
 
   describe('#terminate', () => {