Repository: websockets/ws (js)

Issue:
WebSocket.close() does not close the connection
```
WebSocket.prototype.close = function(code, data) {
  if (this.readyState == WebSocket.CLOSING || this.readyState == WebSocket.CLOSED) return;
  if (this.readyState == WebSocket.CONNECTING) {
    this.readyState = WebSocket.CLOSED;
    return;
  }
```

So, if I create a client WebSocket and, while connecting, I call `close()` on it then it does nothing. Instead the handshake continues.

I see that `http.Agent` is in use and I do not know how it works. Anyhow I expect that a WebSocket client should be able to close/abort the TCP connection at any time once the GET has been sent and before the 101 arrives.


Pull request description:
Abort the request if `close` is called while connecting
Currently `WebSocket.prototype.close()` only sets the `readyState` to [`CLOSED`](https://github.com/websockets/ws/blob/8aa65e2f4da33083d1c20e46240aacc4fe40822c/lib/WebSocket.js#L267) if called before receiving a handshake response. Then the connection is eventually closed when the response is [received](https://github.com/websockets/ws/blob/8aa65e2f4da33083d1c20e46240aacc4fe40822c/lib/WebSocket.js#L633-L639).
This is wrong as the `connection` event is still emitted on the server.

This patch aborts the client request if `WebSocket.prototype.close()` is called before receiving a handshake response.

Fixes #388

Patch:
diff --git a/lib/WebSocket.js b/lib/WebSocket.js
index 1abbb740..0da5c0f7 100644
--- a/lib/WebSocket.js
+++ b/lib/WebSocket.js
@@ -262,10 +262,10 @@ class WebSocket extends EventEmitter {
    */
   close (code, data) {
     if (this.readyState === WebSocket.CLOSED) return;
-
     if (this.readyState === WebSocket.CONNECTING) {
-      this.readyState = WebSocket.CLOSED;
-      return;
+      this._req.abort();
+      this.emit('error', new Error('closed before the connection is established'));
+      return this.finalize(true);
     }
 
     if (this.readyState === WebSocket.CLOSING) {
@@ -374,6 +374,11 @@ class WebSocket extends EventEmitter {
    */
   terminate () {
     if (this.readyState === WebSocket.CLOSED) return;
+    if (this.readyState === WebSocket.CONNECTING) {
+      this._req.abort();
+      this.emit('error', new Error('closed before the connection is established'));
+      return this.finalize(true);
+    }
 
     if (this._socket) {
       this.readyState = WebSocket.CLOSING;
@@ -385,8 +390,6 @@ class WebSocket extends EventEmitter {
       //
       clearTimeout(this._closeTimer);
       this._closeTimer = setTimeout(this._finalize, closeTimeout, true);
-    } else if (this.readyState === WebSocket.CONNECTING) {
-      this.finalize(true);
     }
   }
 }
@@ -618,43 +621,32 @@ function initAsClient (address, protocols, options) {
 
   if (agent) requestOptions.agent = agent;
 
-  const req = httpObj.get(requestOptions);
+  this._req = httpObj.get(requestOptions);
+
+  this._req.on('error', (error) => {
+    if (this._req.aborted) return;
 
-  req.on('error', (error) => {
     this.emit('error', error);
-    this.finalize(error);
+    this.finalize(true);
   });
 
-  req.on('response', (res) => {
-    var error;
-
-    if (!this.emit('unexpected-response', req, res)) {
-      error = new Error(`unexpected server response (${res.statusCode})`);
-      req.abort();
-      this.emit('error', error);
+  this._req.on('response', (res) => {
+    if (!this.emit('unexpected-response', this._req, res)) {
+      this._req.abort();
+      this.emit('error', new Error(`unexpected server response (${res.statusCode})`));
+      this.finalize(true);
     }
-
-    this.finalize(error);
   });
 
-  req.on('upgrade', (res, socket, head) => {
-    if (this.readyState === WebSocket.CLOSED) {
-      // client closed before server accepted connection
-      this.emit('close');
-      this.removeAllListeners();
-      socket.end();
-      return;
-    }
-
+  this._req.on('upgrade', (res, socket, head) => {
     const digest = crypto.createHash('sha1')
       .update(key + GUID, 'binary')
       .digest('base64');
 
     if (res.headers['sec-websocket-accept'] !== digest) {
+      socket.destroy();
       this.emit('error', new Error('invalid server key'));
-      this.removeAllListeners();
-      socket.end();
-      return;
+      return this.finalize(true);
     }
 
     const serverProt = res.headers['sec-websocket-protocol'];
@@ -670,30 +662,28 @@ function initAsClient (address, protocols, options) {
     }
 
     if (protError) {
+      socket.destroy();
       this.emit('error', new Error(protError));
-      this.removeAllListeners();
-      socket.end();
-      return;
+      return this.finalize(true);
     }
 
     if (serverProt) this.protocol = serverProt;
 
     const serverExtensions = Extensions.parse(res.headers['sec-websocket-extensions']);
+
     if (perMessageDeflate && serverExtensions[PerMessageDeflate.extensionName]) {
       try {
         perMessageDeflate.accept(serverExtensions[PerMessageDeflate.extensionName]);
       } catch (err) {
+        socket.destroy();
         this.emit('error', new Error('invalid extension parameter'));
-        this.removeAllListeners();
-        socket.end();
-        return;
+        return this.finalize(true);
       }
+
       this.extensions[PerMessageDeflate.extensionName] = perMessageDeflate;
     }
 
+    this._req = null;
     this.setSocket(socket, head);
-
-    req.removeAllListeners();
-    agent = null;
   });
 }


Test patch:
diff --git a/test/WebSocket.test.js b/test/WebSocket.test.js
index 242fb294..debaf5d0 100644
--- a/test/WebSocket.test.js
+++ b/test/WebSocket.test.js
@@ -95,39 +95,28 @@ describe('WebSocket', function () {
       wss.on('connection', (ws) => ws.send('foobar'));
     });
 
-    it('#url exposes the server url', function (done) {
-      server.createServer(++port, (srv) => {
-        const url = `ws://localhost:${port}`;
-        const ws = new WebSocket(url);
-
-        assert.strictEqual(ws.url, url);
+    it('#url exposes the server url', function () {
+      const url = `ws://localhost:${port}`;
+      const ws = new WebSocket(url, { agent: new CustomAgent() });
 
-        ws.on('close', () => srv.close(done));
-        ws.close();
-      });
+      assert.strictEqual(ws.url, url);
     });
 
-    it('#protocolVersion exposes the protocol version', function (done) {
-      server.createServer(++port, (srv) => {
-        const ws = new WebSocket(`ws://localhost:${port}`);
-
-        assert.strictEqual(ws.protocolVersion, 13);
-
-        ws.on('close', () => srv.close(done));
-        ws.close();
+    it('#protocolVersion exposes the protocol version', function () {
+      const ws = new WebSocket(`ws://localhost:${port}`, {
+        agent: new CustomAgent()
       });
+
+      assert.strictEqual(ws.protocolVersion, 13);
     });
 
     describe('#bufferedAmount', function () {
-      it('defaults to zero', function (done) {
-        server.createServer(++port, (srv) => {
-          const ws = new WebSocket(`ws://localhost:${port}`);
-
-          assert.strictEqual(ws.bufferedAmount, 0);
-
-          ws.on('close', () => srv.close(done));
-          ws.close();
+      it('defaults to zero', function () {
+        const ws = new WebSocket(`ws://localhost:${port}`, {
+          agent: new CustomAgent()
         });
+
+        assert.strictEqual(ws.bufferedAmount, 0);
       });
 
       it('defaults to zero upon "open"', function (done) {
@@ -203,15 +192,12 @@ describe('WebSocket', function () {
     });
 
     describe('#readyState', function () {
-      it('defaults to connecting', function (done) {
-        server.createServer(++port, (srv) => {
-          const ws = new WebSocket(`ws://localhost:${port}`);
-
-          assert.strictEqual(ws.readyState, WebSocket.CONNECTING);
-
-          ws.on('close', () => srv.close(done));
-          ws.close();
+      it('defaults to connecting', function () {
+        const ws = new WebSocket(`ws://localhost:${port}`, {
+          agent: new CustomAgent()
         });
+
+        assert.strictEqual(ws.readyState, WebSocket.CONNECTING);
       });
 
       it('set to open once connection is established', function (done) {
@@ -236,7 +222,7 @@ describe('WebSocket', function () {
             srv.close(done);
           });
 
-          ws.close(1001);
+          ws.on('open', () => ws.close(1001));
         });
       });
 
@@ -249,7 +235,7 @@ describe('WebSocket', function () {
             srv.close(done);
           });
 
-          ws.terminate();
+          ws.on('open', () => ws.terminate());
         });
       });
     });
@@ -308,26 +294,68 @@ describe('WebSocket', function () {
   });
 
   describe('connection establishing', function () {
-    it('can disconnect before connection is established', function (done) {
-      server.createServer(++port, (srv) => {
+    it('can terminate before connection is established (1/2)', function (done) {
+      const wss = new WebSocketServer({ port: ++port }, () => {
         const ws = new WebSocket(`ws://localhost:${port}`);
 
         ws.on('open', () => assert.fail(null, null, 'connect shouldnt be raised here'));
-        ws.on('close', () => srv.close(done));
+        ws.on('error', (err) => {
+          assert.ok(err instanceof Error);
+          assert.strictEqual(err.message, 'closed before the connection is established');
+          ws.on('close', () => wss.close(done));
+        });
         ws.terminate();
       });
     });
 
-    it('can close before connection is established', function (done) {
-      server.createServer(++port, (srv) => {
+    it('can terminate before connection is established (2/2)', function (done) {
+      const wss = new WebSocketServer({
+        verifyClient: (info, cb) => setTimeout(cb, 300, true),
+        port: ++port
+      }, () => {
         const ws = new WebSocket(`ws://localhost:${port}`);
 
         ws.on('open', () => assert.fail(null, null, 'connect shouldnt be raised here'));
-        ws.on('close', () => srv.close(done));
+        ws.on('error', (err) => {
+          assert.ok(err instanceof Error);
+          assert.strictEqual(err.message, 'closed before the connection is established');
+          ws.on('close', () => wss.close(done));
+        });
+        setTimeout(() => ws.terminate(), 150);
+      });
+    });
+
+    it('can close before connection is established (1/2)', function (done) {
+      const wss = new WebSocketServer({ port: ++port }, () => {
+        const ws = new WebSocket(`ws://localhost:${port}`);
+
+        ws.on('open', () => assert.fail(null, null, 'connect shouldnt be raised here'));
+        ws.on('error', (err) => {
+          assert.ok(err instanceof Error);
+          assert.strictEqual(err.message, 'closed before the connection is established');
+          ws.on('close', () => wss.close(done));
+        });
         ws.close(1001);
       });
     });
 
+    it('can close before connection is established (2/2)', function (done) {
+      const wss = new WebSocketServer({
+        verifyClient: (info, cb) => setTimeout(cb, 300, true),
+        port: ++port
+      }, () => {
+        const ws = new WebSocket(`ws://localhost:${port}`);
+
+        ws.on('open', () => assert.fail(null, null, 'connect shouldnt be raised here'));
+        ws.on('error', (err) => {
+          assert.ok(err instanceof Error);
+          assert.strictEqual(err.message, 'closed before the connection is established');
+          ws.on('close', () => wss.close(done));
+        });
+        setTimeout(() => ws.close(1001), 150);
+      });
+    });
+
     it('can handle error before request is upgraded', function (done) {
       // Here, we don't create a server, to guarantee that the connection will
       // fail before the request is upgraded
@@ -803,15 +831,17 @@ describe('WebSocket', function () {
     });
 
     it('before connect should pass error through callback, if present', function (done) {
-      server.createServer(++port, (srv) => {
+      const wss = new WebSocketServer({ port: ++port }, () => {
         const ws = new WebSocket(`ws://localhost:${port}`);
 
-        ws.send('hi', (error) => {
-          assert.ok(error instanceof Error);
-          srv.close(done);
-          ws.terminate();
+        ws.send('hi', (err) => {
+          assert.ok(err instanceof Error);
+          assert.strictEqual(err.message, 'not opened');
+          ws.on('close', () => wss.close(done));
         });
       });
+
+      wss.on('connection', (ws) => ws.close());
     });
 
     it('without data should be successful', function (done) {
diff --git a/test/WebSocketServer.test.js b/test/WebSocketServer.test.js
index cceb53f9..3579dcf8 100644
--- a/test/WebSocketServer.test.js
+++ b/test/WebSocketServer.test.js
@@ -202,10 +202,9 @@ describe('WebSocketServer', function () {
         const ws = new WebSocket(`ws://localhost:${port}`);
       });
 
-      wss.on('connection', (client) => {
+      wss.on('connection', (ws) => {
         assert.strictEqual(wss.clients.size, 1);
-        wss.close();
-        done();
+        wss.close(done);
       });
     });
 
@@ -213,12 +212,13 @@ describe('WebSocketServer', function () {
       const wss = new WebSocketServer({ port: ++port, clientTracking: false }, () => {
         assert.strictEqual(wss.clients, undefined);
         const ws = new WebSocket(`ws://localhost:${port}`);
+
+        ws.on('open', () => ws.close());
       });
 
-      wss.on('connection', (client) => {
+      wss.on('connection', (ws) => {
         assert.strictEqual(wss.clients, undefined);
-        wss.close();
-        done();
+        ws.on('close', () => wss.close(done));
       });
     });
 
@@ -226,14 +226,13 @@ describe('WebSocketServer', function () {
       const wss = new WebSocketServer({ port: ++port }, () => {
         const ws = new WebSocket(`ws://localhost:${port}`);
 
-        wss.on('connection', (client) => {
-          client.on('close', () => {
-            assert.strictEqual(wss.clients.size, 0);
-            wss.close();
-            done();
-          });
+        ws.on('open', () => ws.terminate());
+      });
 
-          ws.close();
+      wss.on('connection', (ws) => {
+        ws.on('close', () => {
+          assert.strictEqual(wss.clients.size, 0);
+          wss.close(done);
         });
       });
     });
@@ -242,14 +241,13 @@ describe('WebSocketServer', function () {
       const wss = new WebSocketServer({ port: ++port }, () => {
         const ws = new WebSocket(`ws://localhost:${port}`);
 
-        wss.on('connection', (client) => {
-          client.on('close', () => {
-            assert.strictEqual(wss.clients.size, 0);
-            wss.close();
-            done();
-          });
+        ws.on('open', () => ws.close());
+      });
 
-          ws.close();
+      wss.on('connection', (ws) => {
+        ws.on('close', () => {
+          assert.strictEqual(wss.clients.size, 0);
+          wss.close(done);
         });
       });
     });