http: added closeAllConnections and closeIdleConnections to http.server

[ShogunPanda]

This PR introduces two new methods on http.Server and https.Server:

1. closeAllConnections(): Closes all sockets connected to the server.
2. closeIdleConnections(): Closes all sockets connected to the server which are not actively sending a request or waiting for a response.

Fixes #41578.

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/net

[mcollina]

lgtm

[lpinca]

For API consistency I'm -0 on this. http.Server#close(), https.Server#close(), Http2Server#close(), net.Server#close(), tls.Server#close() should all work in the same way. It might not be user friendly but developers can already do what is being proposed here by tracking connections explicitly and closing them manually.

[mscdex]

Isn't the 'keep-alive' value not very useful since non-idle keep-alive connections will still keep the process alive after their requests are finished?

[ShogunPanda]

For API consistency I'm -0 on this. http.Server#close(), https.Server#close(), Http2Server#close(), net.Server#close(), tls.Server#close() should all work in the same way. It might not be user friendly but developers can already do what is being proposed here by tracking connections explicitly and closing them manually.

Well, the connections list is not exposed to the developer, so they can't really do that.

I agree that all the HTTP* servers should behave the same (and it could come in a future PR for Http2Server), while net.Server and tls.Server serve a different purpose so API parity I don't think is applicable here.

[ShogunPanda]

Isn't the 'keep-alive' value not very useful since non-idle keep-alive connections will still keep the process alive after their requests are finished?

Actually it is bug in the implementation - Once non-idle requests have been served, they have to be evicted as well. Thanks for noticing it, I'll update this shortly.

[lpinca]

Well, the connections list is not exposed to the developer, so they can't really do that.

All servers emit the 'connection' / 'secureConnection' events. The connection list can be built by the developer using those events.

[ShogunPanda]

Well, the connections list is not exposed to the developer, so they can't really do that.

All servers emit the 'connection' / 'secureConnection' events. The connection list can be built by the developer using those events.

I see, make sense. I thought you were talking about the new data structure I recently introduced.

[mcollina]

Well, the connections list is not exposed to the developer, so they can't really do that.

All servers emit the 'connection' / 'secureConnection' events. The connection list can be built by the developer using those events.

Unfortunately that introduces a major overhead that makes it unfeasible in userland. @ShogunPanda or @jsumners could produce more evidence of this but we have researched this extensively.

[lpinca]

Unfortunately that introduces a major overhead that makes it unfeasible in userland. @ShogunPanda or @jsumners could produce more evidence of this but we have researched this extensively.

I'm interested because it only takes a set of references to already existing objects. I would be surprised if that turns out to be a major overhead.

[jsumners]

I believe this PR is a direct result of:

• Draining connections on close fastify/fastify#3617
• Add keep-alive connection tracking and reaping (resolve #3617) fastify/fastify#3619

The primary issue that impacts performance is retaining references to the requests associated with the sockets. Ideally, this feature would let any active requests finish and then reap the remaining sockets. Currently, Node waits for all sockets to close before terminating even if there are no pending requests on those sockets.

According to information in the above links, information about pending requests is not publicly available from the socket reference. So it isn't possible to determine if any requests are pending in "user land" without also keeping track of the requests. The force close implementation in Fastify ignores this and terminates all sockets regardless of whether or not any requests are currently active. This is a poor implementation, but was the only remotely performant way to get it done.

[lpinca]

According to information in the above links, information about pending requests is not publicly available from the socket reference. So it isn't possible to determine if any requests are pending in "user land" without also keeping track of the requests.

I understand but in this case the patch would only be useful if the force option is set to 'keep-alive' and there is no guarantee that the process will exit because there might be other open connections like WebSocket connections. The other option value would be equivalent to something like this:

const http = require('http');

const sockets = new Set();

const server = http.createServer();

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

function socketOnClose() {
  sockets.delete(this);
}

server.close();

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

[ShogunPanda]

@lpinca Yes, for the all case you are right. Even though taking advantage of the node internal is obviously faster than using EventEmitter and introduces less overhead.

[ShogunPanda]

About the WebSocket objections, you are also right. But we assume developers know what they are doing :)

The keep-alive option is useful to let current pending request to be handled but without accepting any new request on already opened connections (which is possible as of today).

[ShogunPanda]

As a different solution to achieve this, I might leave close as it is now and add the following new methods to http.Server (and https.Server):

1. server.closeConnections([type]), which performs the same new logic I just added to .close
2. connections, activeConnections, idleConnectins and expiredCollections, which make the various connections list readable from the developer.

The reason for 1 is to being able to solve the issue without changing existing functionality.

The reason for 2 is to enable developer to do custom connections handling without having to trace them via EE.

What do you think?

[mcollina]

Will exposing the connection lists be enough to implement this feature in userland?

[ShogunPanda]

Absolutely. The other method would just become a fancy for loop.

[mcollina]

Then let's expose the lists.

[ShogunPanda]

Done.

@nodejs/http @lpinca Can you please re-review?

[jsumners]

A request is not a connection. A request is a communication across a connection. I think server.activeConnections is misleading. According to the subsequent docs, it is server.activeRequests.

[ShogunPanda]

Nope, it's connections. I return the sockets, even though the way the sockets are categorized is given by whether there is an active/expired request being sent through it.

[jsumners]

How does this help with draining connections? The socket does not expose the requests associated with it.

[ShogunPanda]

I see your point now.
Let me clarify that, of all these methods, only allConnections and idleConnections are strictly needed to properly implement the forced close logic.

But I do see your point and, on a second thought, it makes sense to expose the requests rather than socket. This way the developer has all the needed informations to implement any logic they might want.

[jsumners]

My point is that requests are typically short lived. Whereas connections can be open for literal wall clock minutes. What Node.js currently supports is:

• .close stops listening for new connections but waits for current connections to end before the process can exit. This scenario allows new requests on existing connections until they timeout.

There are two other scenarios that can/should be supported:

1. Terminate all connections regardless of pending/active requests such that the process can terminate. This is the scenario Fastify is currently implementing.
2. Stop listening for new connections and stop accepting new requests while allowing current requests to finish and subsequently terminating the associated connections.

We have seen other communities provide scenario 1, so maybe 2 isn't feasible. I don't know.

[ShogunPanda]

#2 is hard as it needs a check every time a request is finished. Which will impact performance. So eventually (which is the intention of this PR) we go with 1 (as suggested in my last comment).

[jsumners]

I'm not clear what this provides. If a request is expired, that means Node is no longer tracking it, correct? Similarly, if a connection has expired it should have also been removed by Node, yes? So what utility does this method provide?

[ShogunPanda]

You are 90% right. The thing is that the Node evicts expired connection every http.Server.connectionsCheckingInterval milliseconds (default is 30s), so it can definitely happen that connections are expired but not evicted yet. Hence this method to eventually allow the user to do other things (like log these connections before evictions or similar).

[tniessen]

Constructing potentially large lists of resources generally feels like an anti-pattern in Node.js, and unless the application code is entirely synchronous, it is prone to race conditions.

[juanarbol]

Some parts of this rely on top of #41263, I did not find a way to rapidly backport this

[ShogunPanda]

@juanarbol Unfortunately I don't think it's possible without backporting both PR. The problem is that #41263 is semver-major

[juanarbol]

That's a shame 💔

[SimenB]

Sorry for posting on an old issue, but was #42812 (comment) ever done? Running git blame on the linked line of code seems to say "no" (unchanged for 14 months)

[ShogunPanda]

@SimenB No, it was not done yet. I'll fix it soon.