[major] Do not re-emit net.Socket errors

Author: lpinca

Diff for: doc/ws.md

@@ -244,8 +244,7 @@ human-readable string explaining why the connection has been closed.
 
 - `error` {Error}
 
-Emitted when an error occurs. Errors from the underlying `net.Socket` are
-forwarded here.
+Emitted when an error occurs.
 
 ### Event: 'message'
 

Diff for: lib/websocket.js

@@ -17,7 +17,6 @@ const readyStates = ['CONNECTING', 'OPEN', 'CLOSING', 'CLOSED'];
 const kWebSocket = constants.kWebSocket;
 const protocolVersions = [8, 13];
 const closeTimeout = 30 * 1000; // Allow 30 seconds to terminate the connection cleanly.
-const NOOP = constants.NOOP;
 
 /**
  * Class representing a WebSocket.
@@ -49,7 +48,6 @@ class WebSocket extends EventEmitter {
     this._receiver = null;
     this._sender = null;
     this._socket = null;
-    this._error = null;
 
     if (address !== null) {
       if (!protocols) {
@@ -132,10 +130,10 @@ class WebSocket extends EventEmitter {
     receiver[kWebSocket] = this;
     socket[kWebSocket] = this;
 
-    receiver.on('message', receiverOnMessage);
     receiver.on('close', receiverOnClose);
     receiver.on('drain', receiverOnDrain);
     receiver.on('error', receiverOnError);
+    receiver.on('message', receiverOnMessage);
     receiver.on('ping', receiverOnPing);
     receiver.on('pong', receiverOnPong);
 
@@ -148,7 +146,6 @@ class WebSocket extends EventEmitter {
     socket.on('data', socketOnData);
     socket.on('end', socketOnEnd);
     socket.on('error', socketOnError);
-    socket.on('error', NOOP);
 
     this.readyState = WebSocket.OPEN;
     this.emit('open');
@@ -167,17 +164,11 @@ class WebSocket extends EventEmitter {
       return;
     }
 
-    const err = this._error;
-
-    if (err) {
-      this._error = null;
-      this.emit('error', err);
-    }
-
     if (this._extensions[PerMessageDeflate.extensionName]) {
       this._extensions[PerMessageDeflate.extensionName].cleanup();
     }
 
+    this._receiver.removeAllListeners();
     this.emit('close', this._closeCode, this._closeMessage);
   }
 
@@ -359,7 +350,6 @@ class WebSocket extends EventEmitter {
 
     if (this._socket) {
       this.readyState = WebSocket.CLOSING;
-      this._socket.removeListener('error', socketOnError);
       this._socket.destroy();
     }
   }
@@ -726,15 +716,21 @@ function receiverOnDrain () {
 function receiverOnError (err) {
   const websocket = this[kWebSocket];
 
-  if (websocket._error) return;
-
   websocket.readyState = WebSocket.CLOSING;
-  websocket._socket.removeListener('error', socketOnError);
   websocket._closeCode = err[constants.kStatusCode];
   websocket.emit('error', err);
   websocket._socket.destroy();
 }
 
+/**
+ * The listener of the `Receiver` `'finish'` event.
+ *
+ * @private
+ */
+function receiverOnFinish () {
+  this[kWebSocket].emitClose();
+}
+
 /**
  * The listener of the `Receiver` `'message'` event.
  *
@@ -754,7 +750,7 @@ function receiverOnMessage (data) {
 function receiverOnPing (data) {
   const websocket = this[kWebSocket];
 
-  websocket.pong(data, !websocket._isServer, NOOP);
+  websocket.pong(data, !websocket._isServer, constants.NOOP);
   websocket.emit('ping', data);
 }
 
@@ -776,7 +772,8 @@ function receiverOnPong (data) {
 function socketOnClose () {
   const websocket = this[kWebSocket];
 
-  this.removeListener('error', socketOnError);
+  this.removeListener('close', socketOnClose);
+  this.removeListener('data', socketOnData);
   this.removeListener('end', socketOnEnd);
   this[kWebSocket] = undefined;
 
@@ -799,13 +796,8 @@ function socketOnClose () {
   ) {
     websocket.emitClose();
   } else {
-    const emitClose = () => {
-      websocket._receiver.removeAllListeners();
-      websocket.emitClose();
-    };
-
-    websocket._receiver.on('error', emitClose);
-    websocket._receiver.on('finish', emitClose);
+    websocket._receiver.on('error', receiverOnFinish);
+    websocket._receiver.on('finish', receiverOnFinish);
   }
 }
 
@@ -830,26 +822,23 @@ function socketOnEnd () {
   const websocket = this[kWebSocket];
 
   websocket.readyState = WebSocket.CLOSING;
-  this.removeListener('error', socketOnError);
   websocket._receiver.end();
   this.end();
 }
 
 /**
  * The listener of the `net.Socket` `'error'` event.
  *
- * @param {Error} err The emitted error
  * @private
  */
-function socketOnError (err) {
+function socketOnError () {
   const websocket = this[kWebSocket];
 
-  websocket.readyState = WebSocket.CLOSING;
   this.removeListener('error', socketOnError);
+  this.on('error', constants.NOOP);
 
-  //
-  // There might be valid buffered data in the socket waiting to be read so we
-  // can't re-emit this error immediately.
-  //
-  websocket._error = err;
+  if (websocket) {
+    websocket.readyState = WebSocket.CLOSING;
+    this.destroy();
+  }
 }

Diff for: test/websocket.test.js

@@ -8,7 +8,6 @@ const crypto = require('crypto');
 const https = require('https');
 const http = require('http');
 const dns = require('dns');
-const net = require('net');
 const fs = require('fs');
 const os = require('os');
 
@@ -393,6 +392,28 @@ describe('WebSocket', function () {
       });
     });
 
+    it('does not re-emit `net.Socket` errors', function (done) {
+      const wss = new WebSocket.Server({ port: 0 }, () => {
+        const ws = new WebSocket(`ws://localhost:${wss.address().port}`);
+
+        ws.on('open', () => {
+          ws._socket.on('error', (err) => {
+            assert.ok(err instanceof Error);
+            assert.ok(err.message.startsWith('write E'));
+            ws.on('close', (code, message) => {
+              assert.strictEqual(message, '');
+              assert.strictEqual(code, 1006);
+              wss.close(done);
+            });
+          });
+
+          for (const client of wss.clients) client.terminate();
+          ws.send('foo');
+          ws.send('bar');
+        });
+      });
+    });
+
     it("emits an 'upgrade' event", function (done) {
       const wss = new WebSocket.Server({ port: 0 }, () => {
         const ws = new WebSocket(`ws://localhost:${wss.address().port}`);
@@ -1160,36 +1181,6 @@ describe('WebSocket', function () {
       });
     });
 
-    it('emits an error if the close frame can not be sent', function (done) {
-      const wss = new WebSocket.Server({ port: 0 }, () => {
-        const socket = net.createConnection(wss.address().port, () => {
-          socket.write(
-            'GET / HTTP/1.1\r\n' +
-            'Host: localhost\r\n' +
-            'Upgrade: websocket\r\n' +
-            'Connection: Upgrade\r\n' +
-            'Sec-WebSocket-Key: qqFVFwaCnSMXiqfezY/AZQ==\r\n' +
-            'Sec-WebSocket-Version: 13\r\n' +
-            '\r\n'
-          );
-          socket.destroy();
-        });
-
-        wss.on('connection', (ws) => {
-          ws.on('error', (err) => {
-            assert.ok(err instanceof Error);
-            assert.ok(err.message.startsWith('write E'));
-            ws.on('close', (code, message) => {
-              assert.strictEqual(message, '');
-              assert.strictEqual(code, 1006);
-              wss.close(done);
-            });
-          });
-          ws.close();
-        });
-      });
-    });
-
     it('sends the close status code only when necessary', function (done) {
       let sent;
       const wss = new WebSocket.Server({ port: 0 }, () => {