[minor] Simplify the closing handshake

- When the socket emits the `'end'` event, do not call `socket.end()`.
  End only the `receiver` stream.
- Do not wait for a close frame to be received and sent before calling
  `socket.end()`. Call it right after the close frame is sent.
- When the `receiver` stream emits `'finish'`, send a close frame if no
  close frame is received.

The assumption is that the socket is allowed to be half-open. On the
server side this is always true (unless the user explicitly sets the
`allowHalfOpen` property of the socket to `false`). On the client side
the user might use an agent so we set `socket.allowHalfOpen` to `true`
when the `http.ClientRequest` object emits the `'upgrade'` event.

Refs: #1899

Author: lpinca

lib/websocket.js

@@ -123,7 +123,7 @@ class WebSocket extends EventEmitter {
    * @type {Number}
    */
   get readyState() {
-    return this._readyState;
+    return this._readyState === -2 ? WebSocket.CLOSING : this._readyState;
   }
 
   /**
@@ -159,6 +159,7 @@ class WebSocket extends EventEmitter {
     receiver.on('conclude', receiverOnConclude);
     receiver.on('drain', receiverOnDrain);
     receiver.on('error', receiverOnError);
+    receiver.on('finish', receiverOnFinish);
     receiver.on('message', receiverOnMessage);
     receiver.on('ping', receiverOnPing);
     receiver.on('pong', receiverOnPong);
@@ -201,31 +202,25 @@ class WebSocket extends EventEmitter {
   /**
    * Start a closing handshake.
    *
-   *          +----------+   +-----------+   +----------+
-   *     - - -|ws.close()|-->|close frame|-->|ws.close()|- - -
-   *    |     +----------+   +-----------+   +----------+     |
-   *          +----------+   +-----------+         |
-   * CLOSING  |ws.close()|<--|close frame|<--+-----+       CLOSING
-   *          +----------+   +-----------+   |
-   *    |           |                        |   +---+        |
-   *                +------------------------+-->|fin| - - - -
-   *    |         +---+                      |   +---+
-   *     - - - - -|fin|<---------------------+
-   *              +---+
-   *
    * @param {Number} [code] Status code explaining why the connection is closing
    * @param {String} [data] A string explaining why the connection is closing
    * @public
    */
   close(code, data) {
-    if (this.readyState === WebSocket.CLOSED) return;
     if (this.readyState === WebSocket.CONNECTING) {
       const msg = 'WebSocket was closed before the connection was established';
       return abortHandshake(this, this._req, msg);
     }
 
-    if (this.readyState === WebSocket.CLOSING) {
-      if (this._closeFrameSent && this._closeFrameReceived) this._socket.end();
+    //
+    // `this._readyState` below is not a typo. The
+    // `WebSocket.prototype.readyState` getter returns `WebSocket.CLOSING` even
+    // if `this._readyState` is `-2`.
+    //
+    if (
+      this._readyState === WebSocket.CLOSING ||
+      this.readyState === WebSocket.CLOSED
+    ) {
       return;
     }
 
@@ -238,7 +233,7 @@ class WebSocket extends EventEmitter {
       if (err) return;
 
       this._closeFrameSent = true;
-      if (this._closeFrameReceived) this._socket.end();
+      this._socket.end();
     });
 
     //
@@ -610,6 +605,15 @@ function initAsClient(websocket, address, protocols, options) {
   });
 
   req.on('upgrade', (res, socket, head) => {
+    //
+    // `tls.connect()` does not support the `allowHalfOpen` option in Node.js <
+    // 12.9.0. Also, the socket creation is not always under our control as the
+    // user might use the `http{,s}.request()` `agent` option and by default
+    // `net.connect()` and `tls.connect()` create a socket not allowed to be
+    // half-open.
+    //
+    socket.allowHalfOpen = true;
+
     websocket.emit('upgrade', res);
 
     //
@@ -679,6 +683,7 @@ function initAsClient(websocket, address, protocols, options) {
  * @private
  */
 function netConnect(options) {
+  options.allowHalfOpen = true;
   options.path = options.socketPath;
   return net.connect(options);
 }
@@ -691,6 +696,7 @@ function netConnect(options) {
  * @private
  */
 function tlsConnect(options) {
+  options.allowHalfOpen = true;
   options.path = undefined;
 
   if (!options.servername && options.servername !== '') {
@@ -820,6 +826,17 @@ function receiverOnError(err) {
  * @private
  */
 function receiverOnFinish() {
+  const websocket = this[kWebSocket];
+
+  if (!websocket._closeFrameReceived) websocket.close();
+}
+
+/**
+ * A listener for the `Receiver` `'error'` or `'finish'` event.
+ *
+ * @private
+ */
+function receiverOnErrorOrFinish() {
   this[kWebSocket].emitClose();
 }
 
@@ -893,8 +910,8 @@ function socketOnClose() {
   ) {
     websocket.emitClose();
   } else {
-    websocket._receiver.on('error', receiverOnFinish);
-    websocket._receiver.on('finish', receiverOnFinish);
+    websocket._receiver.on('error', receiverOnErrorOrFinish);
+    websocket._receiver.on('finish', receiverOnErrorOrFinish);
   }
 }
 
@@ -918,9 +935,13 @@ function socketOnData(chunk) {
 function socketOnEnd() {
   const websocket = this[kWebSocket];
 
-  websocket._readyState = WebSocket.CLOSING;
+  //
+  // This is a special state. It indicates that the connection is going through
+  // the closing handshake but unlike `WebSocket.CLOSING` it does not prevent
+  // `WebSocket.prototype.close()` from sending a close frame.
+  //
+  websocket._readyState = -2;
   websocket._receiver.end();
-  this.end();
 }
 
 /**

test/create-websocket-stream.test.js

@@ -203,7 +203,7 @@ describe('createWebSocketStream', () => {
     });
 
     it('reemits errors', (done) => {
-      let duplexCloseEventEmitted = false;
+      let closeEventCount = 0;
       const wss = new WebSocket.Server({ port: 0 }, () => {
         const ws = new WebSocket(`ws://localhost:${wss.address().port}`);
         const duplex = createWebSocketStream(ws);
@@ -217,18 +217,18 @@ describe('createWebSocketStream', () => {
           );
 
           duplex.on('close', () => {
-            duplexCloseEventEmitted = true;
+            if (++closeEventCount === 2) wss.close(done);
           });
         });
       });
 
       wss.on('connection', (ws) => {
         ws._socket.write(Buffer.from([0x85, 0x00]));
         ws.on('close', (code, reason) => {
-          assert.ok(duplexCloseEventEmitted);
           assert.strictEqual(code, 1002);
           assert.strictEqual(reason, '');
-          wss.close(done);
+
+          if (++closeEventCount === 2) wss.close(done);
         });
       });
     });

test/websocket.test.js

@@ -429,7 +429,7 @@ describe('WebSocket', () => {
 
   describe('Events', () => {
     it("emits an 'error' event if an error occurs", (done) => {
-      let clientCloseEventEmitted = false;
+      let closeEventCount = 0;
       const wss = new WebSocket.Server({ port: 0 }, () => {
         const ws = new WebSocket(`ws://localhost:${wss.address().port}`);
 
@@ -442,19 +442,20 @@ describe('WebSocket', () => {
           );
 
           ws.on('close', (code, reason) => {
-            clientCloseEventEmitted = true;
             assert.strictEqual(code, 1006);
             assert.strictEqual(reason, '');
+
+            if (++closeEventCount === 2) wss.close(done);
           });
         });
       });
 
       wss.on('connection', (ws) => {
         ws.on('close', (code, reason) => {
-          assert.ok(clientCloseEventEmitted);
           assert.strictEqual(code, 1002);
           assert.strictEqual(reason, '');
-          wss.close(done);
+
+          if (++closeEventCount === 2) wss.close(done);
         });
 
         ws._socket.write(Buffer.from([0x85, 0x00]));
@@ -547,6 +548,7 @@ describe('WebSocket', () => {
           .update(req.headers['sec-websocket-key'] + GUID)
           .digest('base64');
 
+        socket.resume();
         socket.end(
           'HTTP/1.1 101 Switching Protocols\r\n' +
             'Upgrade: websocket\r\n' +
@@ -1419,16 +1421,16 @@ describe('WebSocket', () => {
     });
 
     it('honors the `mask` option', (done) => {
-      let serverClientCloseEventEmitted = false;
+      let closeEventCount = 0;
       const wss = new WebSocket.Server({ port: 0 }, () => {
         const ws = new WebSocket(`ws://localhost:${wss.address().port}`);
 
         ws.on('open', () => ws.send('hi', { mask: false }));
         ws.on('close', (code, reason) => {
-          assert.ok(serverClientCloseEventEmitted);
           assert.strictEqual(code, 1002);
           assert.strictEqual(reason, '');
-          wss.close(done);
+
+          if (++closeEventCount === 2) wss.close(done);
         });
       });
 
@@ -1450,9 +1452,10 @@ describe('WebSocket', () => {
           );
 
           ws.on('close', (code, reason) => {
-            serverClientCloseEventEmitted = true;
             assert.strictEqual(code, 1006);
             assert.strictEqual(reason, '');
+
+            if (++closeEventCount === 2) wss.close(done);
           });
         });
       });
@@ -2624,7 +2627,9 @@ describe('WebSocket', () => {
         ws.send('foo');
         ws.send('bar');
         ws.send('baz');
-        ws.send('qux', () => ws._socket.end());
+        ws.send('qux', () => {
+          ws.terminate();
+        });
       });
     });
 
@@ -2679,6 +2684,8 @@ describe('WebSocket', () => {
                   'The socket was closed while data was being compressed'
                 );
               });
+
+              ws.terminate();
             });
 
             ws.on('close', () => {
@@ -2687,10 +2694,6 @@ describe('WebSocket', () => {
             });
           }
         );
-
-        wss.on('connection', (ws) => {
-          ws._socket.end();
-        });
       });
     });