Repository navigation
http: server.closeAllConnections does not destroy upgraded (web)sockets #53536
Description
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 2sHow 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.
Activity
lpinca commentedon Jun 22, 2024
I find it reasonable and it was like this even before
server.closeAllConnections()was added. The method (likeserver. 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 commentedon Jun 22, 2024
cc: @ShogunPanda
ShogunPanda commentedon Jun 23, 2024
I agree with @lpinca.
For the record, the connection make into
ConnectionListsince 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#L938as they are not meant to be subjected toconnectionsCheckingIntervalanymore.I do agree we need a similar
closeAllConnectionsAPI innet.Server.lpinca commentedon Jun 23, 2024
I don't think it should be in Node.js core. It is trivial to do it in userland.
robhogan commentedon Jun 23, 2024
Thanks folks.
Docs PR: #53560
lpinca commentedon Jun 25, 2024
Fixed by d7d9278.
Bounded shutdown: end the streams honestly, give short work ten secon…