Unable to properly cleanup underlying socket with close/terminate

[cTn-dev]

Description

Hi Luigi, i am a bit lost if this a valid edge case to report. but i do find it "odd" that reading the payload from the response (while doing such successfully) can result in the underlying socket being left open).

Calling the request.socket.destroy(); after response end event fires bypasses the problem.
Is this behavior expected from your point of view?

Reproducible in:

• version: 7.4.4
• Node.js version(s): v14.16.1
• OS version(s): Fedora 33, 5.11.12-200.fc33.x86_64

Steps to reproduce:

Demo server (returning invalid data on purpose):

let http = require('http');
let server = http.createServer(function (req, res) {
  res.writeHead(502, {'Content-Type': 'text/html'});
  res.write('uhoh\r\n');
  res.end();
});
server.keepAliveTimeout = 0;
server.listen(4987);

Client demonstrating the issue:

const WebSocket = require('ws');

for (let id = 0; id < 100; id++) {
  let ws = new WebSocket('ws:127.0.0.1:4987');

  ws.on('open', () => {});
  ws.on('close', (code, reason) => {
    console.log('close', id, code, reason);
  });
  ws.on('error', (error) => {
    console.log('error', id, error.toString());
  });
  ws.on('unexpected-response', (request, response) => {
    let chunks = [];
    let payload = null;

    response.on('data', (chunk) => chunks.push(chunk));
    response.on('end', () => {
      payload = Buffer.concat(chunks).toString();

      // uncommenting this when ready
      // request.socket.destroy();

      ws.terminate();
    });
  });
}

Expected result:

With the demo client code above, the bottom line is that the node process shuts down on its own.

Actual result:

The demo client code did not shut down because the underlying TCP sockets are left in ESTABLISHED state.
Regardless of calling ws.close() or ws.terminate().

The "trigger" for the above scenario appears to be binding the additional event handlers used to read the response payload (which does succeeds).

You can confirm that the connections are left open by doing netstat -an | grep ':4987' | grep ESTABLISHED | wc -l

[lpinca]

It is indeed odd but this behavior is defined by Node.js core not ws. Here is a test case using only Node.js core modules.

const http = require('http');
const net = require('net');

const request = http.get({
  port: 4987,
  createConnection: net.connect,
  headers: {
    'Sec-WebSocket-Version': 13,
    'Sec-WebSocket-Key': 'rHat1v1OTJosG31ChyAUVA==',
    Connection: 'Upgrade',
    Upgrade: 'websocket'
  }
});

request.on('response', function (response) {
  response.on('end', function () {
    console.log('done');
    request.abort();
  });

  response.resume();
});

This used to work as expected before nodejs/node@b04e884.

Also, the request does not use an agent so the connection should not be kept alive. However the Connection header is set with a value different than close so the connection is actually kept alive.

[lpinca]

Something like this

diff --git a/lib/websocket.js b/lib/websocket.js
index 0e2a83d..aa47fab 100644
--- a/lib/websocket.js
+++ b/lib/websocket.js
@@ -718,6 +718,11 @@ function abortHandshake(websocket, stream, message) {
 
   if (stream.setHeader) {
     stream.abort();
+
+    if (stream.socket && !stream.socket.destroyed) {
+      stream.socket.destroy();
+    }
+
     stream.once('abort', websocket.emitClose.bind(websocket));
     websocket.emit('error', err);
   } else {

should solve the problem and make the behavior consistent on all supported Node.js versions. It seems a bit hacky but I see no other way without reverting nodejs/node@b04e884 or documenting that ws.close() and ws.terminate() do not close the connection under these circumstances.

What do you think?

[cTn-dev]

I see and yes i do agree, i wasn't aware of the behavior change caused by nodejs/node@b04e884
While slightly hacky, the code/approach above appears to be as clean as it could be and from my point of view is the correct workaround.

The reason i raised this "question" is that "not knowing" the underlying socket could end up being kept alive (which likely includes majority of the users of this library) can have catastrophic consequences in certain scenarios when "rapid reconnect" due to connection failure is necessary.

[lpinca]

I think this is a rare edge case because it depends on:

1. The server sending a "non-upgrade" response with the Connection header set to keep-alive.
2. The user reading the HTTP response body until the 'end' event is emitted.
3. The user calling ws.close() or ws.terminate() after the 'end' event.

so in my opinion it would be sufficient to document that in this case ws.close() or ws.terminate() might not work as expected.

The above patch also has the side effect of preventing the socket from being reused, or worse destroying the socket of another request if a keep-alive agent is used. However this is how it worked before Node.js 14.3.0 and consistency is nice.

[cTn-dev]

I agree it is quite rate (one of the reasons why it took me so long to run into this).
I would definitely try to advocate for consistency > extra documentation as i do think (in general) users do expect for the underlying socket/resources to be released when ws.close() or ws.terminate() is called.

To give you a real world example (which is definitely on the rare side):

1. The server responded with a gateway timeout (ALB with all nodes dead behind it, but the connection header is set to keep-alive)
2. The client code read the body until the 'end' event inside the 'unexpected-response' handler (for debugging purposes)
3. The client code retries after a small delay

Eventually the client system OOMed due to holding on to all the underlying sockets/resources.
In my case it was really "not knowing" the behavior or "expecting close/terminate to handle what i needed" that shot me in the foot.

The bottom line is, i am completely fine with either approach/just trying to find a reasonable way to prevent others from running into similar scenario, as the back end server returning "unexpected garbage" due to technical issues can be quite common.