Simplify the closing handshake

lib/sender.js

@@ -151,6 +151,7 @@ class Sender {
       }),
       cb
     );
+    this._socket.end();
   }
 
   /**

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,6 @@ class WebSocket extends EventEmitter {
       if (err) return;
 
       this._closeFrameSent = true;
-      if (this._closeFrameReceived) this._socket.end();
     });
 
     //
@@ -610,6 +604,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 +682,7 @@ function initAsClient(websocket, address, protocols, options) {
  * @private
  */
 function netConnect(options) {
+  options.allowHalfOpen = true;
   options.path = options.socketPath;
   return net.connect(options);
 }
@@ -691,6 +695,7 @@ function netConnect(options) {
  * @private
  */
 function tlsConnect(options) {
+  options.allowHalfOpen = true;
   options.path = undefined;
 
   if (!options.servername && options.servername !== '') {
@@ -820,6 +825,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 +909,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 +934,15 @@ 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.
+  //
+  if (websocket._readyState === WebSocket.OPEN) {
+    websocket._readyState = -2;
+  }
   websocket._receiver.end();
-  this.end();
 }
 
 /**

test/create-websocket-stream.test.js

@@ -204,6 +204,8 @@ describe('createWebSocketStream', () => {
 
     it('reemits errors', (done) => {
       let duplexCloseEventEmitted = false;
+      let serverClientCloseEventEmitted = false;
+
       const wss = new WebSocket.Server({ port: 0 }, () => {
         const ws = new WebSocket(`ws://localhost:${wss.address().port}`);
         const duplex = createWebSocketStream(ws);
@@ -218,17 +220,19 @@ describe('createWebSocketStream', () => {
 
           duplex.on('close', () => {
             duplexCloseEventEmitted = true;
+            if (serverClientCloseEventEmitted) 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);
+
+          serverClientCloseEventEmitted = true;
+          if (duplexCloseEventEmitted) wss.close(done);
         });
       });
     });

test/sender.test.js

@@ -13,9 +13,10 @@ class MockSocket {
     if (write) this.write = write;
   }
 
+  end() {}
   cork() {}
-  write() {}
   uncork() {}
+  write() {}
 }
 
 describe('Sender', () => {

test/websocket-server.test.js

@@ -400,7 +400,9 @@ describe('WebSocketServer', () => {
 
         server.on('upgrade', (req, socket, head) => {
           wss.handleUpgrade(req, socket, head, (ws) => {
-            ws.close();
+            process.nextTick(() => {
+              ws.close();
+            });
           });
           assert.throws(
             () => wss.handleUpgrade(req, socket, head, NOOP),