Repository: websockets/ws (js)

Issue:
ws.terminate() fires the 'close' event 30 secs after termination?
When `ws.terminate()` (`WebSocket.prototype.terminate`) is called, the connection is immediately ended, yet `cleanupWebsocketResources` is called with a timeout of 30 seconds (a time hardcoded for `closeTimeout`)?

`cleanupWebsocketResources` emits the `'close'` event. So terminating a connection, with eventHandlers registered for the `'close'` event, will then trigger these 30 sec after termination.

Is this intentional? If so, why?

If there is something I have misinterpreted/misunderstood, please enlighten me (is `terminate` only supposed to be called internally? In that case, why expose it on `WebSocket `?) - to me it seems weird that:
A connection that is forcefully terminated waits to clean up, and more importantly, waits to fire the `'close'` event as part of the awaited cleanup.

---

For convenience, here is the code I am referring to, at the time of writing:

**Hardcoded timeout :**

```js
var closeTimeout = 30 * 1000; // Allow 30 seconds to terminate the connection cleanly
```

**WebSocket.prototype.terminate :**

```js
/**
 * Immediately shuts down the connection
 *
 * @api public
 */
WebSocket.prototype.terminate = function terminate () {
  if (this.readyState === WebSocket.CLOSED) return;

  if (this._socket) {
    this.readyState = WebSocket.CLOSING;

    // End the connection
    try {
      this._socket.end();
    } catch (e) {
      // Socket error during end() call, so just destroy it right now
      cleanupWebsocketResources.call(this, true);
      return;
    }

    // Add a timeout to ensure that the connection is completely
    // cleaned up within 30 seconds, even if the clean close procedure
    // fails for whatever reason
    // First cleanup any pre-existing timeout from an earlier "terminate" call,
    // if one exists.  Otherwise terminate calls in quick succession will leak timeouts
    // and hold the program open for `closeTimout` time.
    if (this._closeTimer) { clearTimeout(this._closeTimer); }
    this._closeTimer = setTimeout(cleanupWebsocketResources.bind(this, true), closeTimeout);
  } else if (this.readyState === WebSocket.CONNECTING) {
    cleanupWebsocketResources.call(this, true);
  }
};
```


**cleanupWebsocketResources (this.emit('close', ...) is called):**

```js
function cleanupWebsocketResources (error) {
  if (this.readyState === WebSocket.CLOSED) return;

  this.readyState = WebSocket.CLOSED;

  clearTimeout(this._closeTimer);
  this._closeTimer = null;

  // If the connection was closed abnormally (with an error), or if
  // the close control frame was not received then the close code
  // must default to 1006.
  if (error || !this._closeReceived) {
    this._closeCode = 1006;
  }
  this.emit('close', this._closeCode || 1000, this._closeMessage || '');

  if (this._socket) {
    if (this._ultron) this._ultron.destroy();
    this._socket.on('error', function onerror () {
      try {
        this.destroy();
      } catch (e) {}
    });

    try {
      if (!error) this._socket.end();
      else this._socket.destroy();
    } catch (e) { /* Ignore termination errors */ }

    this._socket = null;
    this._ultron = null;
  }

  if (this._sender) {
    this._sender = this._sender.onerror = null;
  }

  if (this._receiver) {
    this._receiver.cleanup();
    this._receiver = null;
  }

  if (this.extensions[PerMessageDeflate.extensionName]) {
    this.extensions[PerMessageDeflate.extensionName].cleanup();
  }

  this.extensions = null;

  this.removeAllListeners();
  this.on('error', function onerror () {}); // catch all errors after this
  delete this._queue;
}
```

Pull request description:
Make `WebSocket#terminate()` destroy the socket
Currently `WebSocket.prototype.terminate()` only sends a FIN packet to the other peer. If `allowHalfOpen` is `true` and the other peer does not call `socket.end()` the connection will only be closed when the timeout [expires](https://github.com/websockets/ws/blob/4a605b9853a36ce53332f3cad87127b47d4d1030/lib/WebSocket.js#L403).

This patch makes `WebSocket.prototype.terminate()` use `socket.destroy()` so the connection is closed immediately even if the other peer fails to work properly.

Fixes #891.

Patch:
diff --git a/doc/ws.md b/doc/ws.md
index 3cc6c0a9..8893edf4 100644
--- a/doc/ws.md
+++ b/doc/ws.md
@@ -406,11 +406,11 @@ Resume the socket
 - `callback` {Function} An optional callback which is invoked when `data` is
   written out.
 
-Sends `data` through the connection.
+Send `data` through the connection.
 
 ### websocket.terminate()
 
-Send a FIN packet to the other peer.
+Forcibly close the connection.
 
 ### websocket.upgradeReq
 
diff --git a/examples/fileapi/server.js b/examples/fileapi/server.js
index 380ce152..4202818a 100644
--- a/examples/fileapi/server.js
+++ b/examples/fileapi/server.js
@@ -34,8 +34,8 @@ function makePathForFile (filePath, prefix, cb) {
     if (error) return cb(error);
     if (pieces.length === 0) return cb(null, incrementalPath);
     incrementalPath += '/' + pieces.shift();
-    fs.exists(incrementalPath, function (exists) {
-      if (!exists) fs.mkdir(incrementalPath, step);
+    fs.access(incrementalPath, function (err) {
+      if (err) fs.mkdir(incrementalPath, step);
       else process.nextTick(step);
     });
   }
diff --git a/lib/WebSocket.js b/lib/WebSocket.js
index 24bd1bff..c4ed744d 100644
--- a/lib/WebSocket.js
+++ b/lib/WebSocket.js
@@ -20,8 +20,8 @@ const constants = require('./Constants');
 const Receiver = require('./Receiver');
 const Sender = require('./Sender');
 
+const protocolVersions = [8, 13];
 const closeTimeout = 30 * 1000; // Allow 30 seconds to terminate the connection cleanly.
-const protocolVersion = 13;
 
 /**
  * Class representing a WebSocket.
@@ -276,7 +276,7 @@ class WebSocket extends EventEmitter {
     }
 
     if (this.readyState === WebSocket.CLOSING) {
-      if (this._closeCode) this.terminate();
+      if (this._closeCode && this._socket) this._socket.end();
       return;
     }
 
@@ -284,9 +284,8 @@ class WebSocket extends EventEmitter {
     this._sender.close(code, data, !this._isServer, (err) => {
       if (err) this.emit('error', err);
 
-      if (this._closeCode) {
-        this.terminate();
-      } else {
+      if (this._socket) {
+        if (this._closeCode) this._socket.end();
         //
         // Ensure that the connection is cleaned up even when the closing
         // handshake fails.
@@ -391,17 +390,7 @@ class WebSocket extends EventEmitter {
       return;
     }
 
-    if (this._socket) {
-      this.readyState = WebSocket.CLOSING;
-      this._socket.end();
-
-      //
-      // Add a timeout to ensure that the connection is completely cleaned up
-      // within 30 seconds, even if the other peer does not send a FIN packet.
-      //
-      clearTimeout(this._closeTimer);
-      this._closeTimer = setTimeout(this._finalize, closeTimeout, true);
-    }
+    this.finalize(true);
   }
 }
 
@@ -504,15 +493,15 @@ function initAsServerClient (req, socket, head, options) {
  */
 function initAsClient (address, protocols, options) {
   options = Object.assign({
+    protocolVersion: protocolVersions[1],
     protocol: protocols.join(','),
     perMessageDeflate: true,
     localAddress: null,
-    protocolVersion,
     headers: null,
+    family: null,
     origin: null,
     agent: null,
     host: null,
-    family: null,
 
     //
     // SSL options.
@@ -527,8 +516,11 @@ function initAsClient (address, protocols, options) {
     ca: null
   }, options);
 
-  if (options.protocolVersion !== 8 && options.protocolVersion !== 13) {
-    throw new Error('unsupported protocol version');
+  if (protocolVersions.indexOf(options.protocolVersion) === -1) {
+    throw new Error(
+      `unsupported protocol version: ${options.protocolVersion} ` +
+      `(supported versions: ${protocolVersions.join(', ')})`
+    );
   }
 
   this.protocolVersion = options.protocolVersion;
diff --git a/package.json b/package.json
index de6bc088..55132234 100644
--- a/package.json
+++ b/package.json
@@ -31,7 +31,9 @@
     "benchmark": "~2.1.2",
     "bufferutil": "~3.0.0",
     "eslint": "~3.17.0",
-    "eslint-config-standard": "~7.0.0",
+    "eslint-config-standard": "~8.0.0-beta.1",
+    "eslint-plugin-import": "~2.2.0",
+    "eslint-plugin-node": "~4.2.0",
     "eslint-plugin-promise": "~3.5.0",
     "eslint-plugin-standard": "~2.1.0",
     "istanbul": "~0.4.5",


Test patch:
diff --git a/test/WebSocket.test.js b/test/WebSocket.test.js
index 867629d8..ff01c07b 100644
--- a/test/WebSocket.test.js
+++ b/test/WebSocket.test.js
@@ -51,7 +51,7 @@ describe('WebSocket', function () {
 
       assert.throws(
         () => new WebSocket('ws://localhost', options),
-        /^Error: unsupported protocol version$/
+        /^Error: unsupported protocol version: 1000 \(supported versions: 8, 13\)$/
       );
     });