Closing handshake is broken when maxPayload is exceeded

[lpinca]

Description

When maxPayloadExceeded is called, the dead flag is set on the Receiver.
This makes the Receiver ignore everything including the final close frame coming from the other peer to complete the closing handshake.
Since the handshake cannot be completed, the connection is only closed when the cleanup timeout expires.

Test case:

'use strict';

const WebSocket = require('ws');
const http = require('http');

const server = http.createServer();
const wss = new WebSocket.Server({
  perMessageDeflate: false,
  clientTracking: false,
  maxPayload: 10,
  server
});

wss.on('connection', (ws) => {
  ws.on('error', (err) => {
    console.log(err.stack);
    ws.close();
    server.close();
  });
  ws.on('message', () => {
    ws.close();
    server.close();
  });
});

server.listen(3000, () => {
  const ws = new WebSocket('ws://localhost:3000');
  ws.on('open', () => ws.send('0123456789'));
});

The process exits only when the cleanup timeout expires.
If the maxPayload option is removed, the process exits immediately as expected.

@LordMajestros, since you have implemented the feature, do you have any idea on how to fix this?

[lpinca]

I've found 2 possible ways to fix this but I'm not sure if they are applicable:

1 - Remove the this._isServer check from the close method as discussed in 7325d01#commitcomment-18261193.

diff --git a/lib/WebSocket.js b/lib/WebSocket.js
index a2f5ff5..767b66a 100644
--- a/lib/WebSocket.js
+++ b/lib/WebSocket.js
@@ -110,7 +110,7 @@ WebSocket.prototype.close = function close(code, data) {
   }

   if (this.readyState === WebSocket.CLOSING) {
-    if (this._closeReceived && this._isServer) {
+    if (this._closeReceived) {
       this.terminate();
     }
     return;
@@ -125,7 +125,7 @@ WebSocket.prototype.close = function close(code, data) {
     this._sender.close(code, data, mask, function(err) {
       if (err) self.emit('error', err);

-      if (self._closeReceived && self._isServer) {
+      if (self._closeReceived) {
         self.terminate();
       } else {
         // ensure that the connection is cleaned up even when no response of closing handshake.

2 - Fake the retrieval of the close frame from the client when maxPayload is exceeded.

diff --git a/lib/WebSocket.js b/lib/WebSocket.js
index a2f5ff5..8b941ac 100644
--- a/lib/WebSocket.js
+++ b/lib/WebSocket.js
@@ -868,6 +868,7 @@ function establishConnection(ReceiverClass, SenderClass, socket, upgradeHead) {
   };

   self._receiver.onerror = function onerror(reason, errorCode) {
+    if (errorCode === 1009) self._closeReceived = true;
     // close the connection when the receiver reports a HyBi error code
     self.close(typeof errorCode !== 'undefined' ? errorCode : 1002, '');
     self.emit('error', (reason instanceof Error) ? reason : (new Error(reason)));

[nkzawa]

When maxPayloadExceeded is called, the dead flag is set on the Receiver.

This sounds weird since we have to do closing handshake even when any errors occurs.
If it's done properly, we don't need to remove this._isServer or something like that IMHO.

[lpinca]

The root of the issue is actually the dead Receiver so I agree with @nkzawa.

[lpinca]

I've just noticed that the issue happens with any error not only when the maximum payload size is exceeded.

Test case:

'use strict';

const WebSocket = require('ws');
const http = require('http');

const server = http.createServer();
const wss = new WebSocket.Server({ server });

wss.on('connection', (ws) => {
  ws.on('error', (err) => {
    console.log(err.stack);
    ws.close();
    server.close();
  });
});

server.listen(3000, () => {
  const ws = new WebSocket('ws://localhost:3000');
  ws.on('open', () => ws._socket.write('foo'));
});