Review requested:

• @nodejs/http
• @nodejs/net

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.

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?

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.

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.

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.

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.

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.

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.

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.

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();
}

@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.

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).

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?

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

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

Then let's expose the lists.

Done.

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

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)

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