http: server.closeAllConnections does not destroy upgraded (web)sockets

[robhogan]

Version

v22.3.0

Platform

Darwin

Subsystem

http

What steps will reproduce the bug?

Running the following script (node script.js) reproduces the bug - the close callback is never invoked and the process does not terminate until forced to with the second (unreffed) timeout.

const http = require("http");

const server = http.createServer(() => {});
server.on("error", console.warn);
server.on("upgrade", (request, socket, head) => {
  socket.write(
    "HTTP/1.1 101 Web Socket Protocol Handshake\r\n" +
      "Upgrade: WebSocket\r\n" +
      "Connection: Upgrade\r\n" +
      "\r\n",
  );
});

function makeUpgradeRequest() {
  console.log("connecting with an upgrade request");
  const req = http.request({
    port: 8081,
    host: "localhost",
    headers: {
      Connection: "Upgrade",
      Upgrade: "websocket",
    },
  });
  req.end();
  req.on("error", console.warn);
  req.on("upgrade", (res, socket, upgradeHead) => {
    console.log("request upgraded");
  });
}

server.listen(8081, () => {
  console.log("server listening");
  makeUpgradeRequest();
});

setTimeout(() => {
  console.log("trying to close...");
  server.close(() => {
    // Reality: this is never called.
    console.log("closed callback");
  });
  // Expectation: this closes the socket, triggering server close.
  server.closeAllConnections();
}, 1000).unref();

setTimeout(() => {
  console.warn("did not close within 2s");
  process.exit(1);
}, 3000).unref();

Output I see:

node script.js
server listening
connecting with an upgrade request
request upgraded
trying to close...
did not close within 2s

How often does it reproduce? Is there a required condition?

Reproduces consistently as far as I can tell.

What is the expected behavior? Why is that the expected behavior?

server.closeAllConnections() is documented:

Closes all connections connected to this server, including active connections connected to this server which are sending a request or waiting for a response.

This is a forceful way of closing all connections and should be used with caution.

There's no suggestion that a websocket would be an exception to that - I'd expect it to destroy those sockets, the server would be drained, and 'close' would be emitted.

What do you see instead?

The server.close() callback is not invoked, the websocket connection stays open.

Additional information

If someone can confirm whether this is a bug or docs issue I'd be happy to work on a fix.

Bit of investigation: It looks like these connections don't make it into ConnectionList (screenshot), which is why closeAllConnections doesn't find them, though they are counted on net's _connections counter (which is used to determine when 'close' is emitted) - maybe on_message_begin isn't fired from llhttp because we're not following usual http request/response? That's as far as I've dug.

[lpinca]

I find it reasonable and it was like this even before server.closeAllConnections() was added. The method (like server. closeIdleConnections()) was added for HTTP keep-alive connections. I agree that the name is misleading and we should clarify in the documentation that it only handles established connections not upgraded to a different protocol.

If the method actually needs to close all connections, then I think it would be better to move it to net.Server.

[lpinca]

cc: @ShogunPanda

[ShogunPanda]

I agree with @lpinca.
For the record, the connection make into ConnectionList since the HTTP parser will parse them up to the headers section.
They are then removed in https://github.com/nodejs/node/blob/main/lib/_http_server.js#L938 as they are not meant to be subjected to connectionsCheckingInterval anymore.

I do agree we need a similar closeAllConnections API in net.Server.

[lpinca]

I do agree we need a similar closeAllConnections API in net.Server.

I don't think it should be in Node.js core. It is trivial to do it in userland.

const sockets = new Set()

function closeAllConnections() {
  for (const socket of sockets) {
    socket.destroy();
  }
}

server.on('connection', function (socket) {
  sockets.add(socket);

  socket.on('close', function () {
    sockets.delete(socket);
  })
});

[robhogan]

Thanks folks.

I agree that the name is misleading and we should clarify in the documentation that it only handles established connections not upgraded to a different protocol.

Docs PR: #53560

[lpinca]

Fixed by d7d9278.