Keep alive connection do not get closed with server.close()

[codygustafson]

The following will remain open as long as the client makes a request within one second intervals making it possible that a server will never gracefully close. With the default of 2 minute timeouts a user or api client would need to stop traffic for two minutes before this will take affect. I propose closing sockets using keep alive immediately after the next request. This will prevent needing to keep track of everything that would need to be closed while making the maximum time to gracefully shutdown fixed(and finite).

var http = require('http');

var server = http.createServer(function(req, res) {
  res.end('test\n');
  server.close()
}).listen(8000, '127.0.0.1');
server.setTimeout(1000);

[brendanashworth]

Thanks for opening an issue. I think a similar solution to this issue is being pursued in #2534.

[kanongil]

Nah, #2534 is about tcp keep alive, while this issue is a http keep alive issue. #2534 is a partial fix for this issue for non-malicious clients, and still imposes an unnecessary timeout delay.

[tshemsedinov]

@kanongil #2534 is about HTTP and HTTPS keep-alive, as indicated in pull-request title http: ... and label. I added separate keepAliveTimeout (default to 5 seconds), but if you want to destroy sockets immediately on server.close() (not waiting for 5 seconds) we need to track each socket (adding it to array and removing after destroy). I do not like this overhead, or you have better solution?

[ronkorving]

Rather than an array, you should probably consider a WeakMap now. It is overhead, but that's still better than a broken feature.

[kanongil]

@tshemsedinov Right you are, though the partial fix part still stands.

The main problem with the tcp timeout fix is that malicious clients (intentional or otherwise) can keep the connection alive by creating new requests on the socket. Eg. when polling for a value every 2 seconds.

[codygustafson]

Just thought I would mention again that the main issue here is that it is possible that sockets never get destroyed. To prevent the overhead @tshemsedinov is talking about, I still think it would be best to do what I mentioned in the description. Which is destroying sockets on the next request to prevent refreshing the timeout for that socket. I think everyone can handle if all sockets are guaranteed to be destroyed within the timeout period.

[ronkorving]

Depends what the timeout period is :) It's been several minutes up till now, right? That's too long for my process to shutdown.

[codygustafson]

Good point.

keepAliveMsecs: {Integer} When using HTTP KeepAlive, how often to send TCP KeepAlive packets over sockets being kept alive. Default = 1000. Only relevant if keepAlive is set to true.

I think this callback would be another good place to karate chop these sockets. That should allow all sockets to be destroyed within a second with no extra overhead.

[tpetry]

This should be fixed or noted in the documentation for server.close([callback]). I have been searching this bug for hours (in a complex application) to research why the http server closes the connection minutes after i requested it to close with no active http request.

[bnoordhuis]

The documentation seems pretty clear to me - "Stops the server from accepting new connections." - but if you think it can be improved, please file a PR with your suggested changes. Do consult CONTRIBUTING.md first, though.

[jinxidoru]

The clarity of the documentation is not the problem. The problem is the fact that this implementation of the http server as it now stands prevents a person from gracefully shutting down an HTTP server.

[bnoordhuis]

Define 'gracefully'? If you mean 'forcibly close client connections', then no, it doesn't do that, and that's deliberate.

OP's suggested change is a no go because it makes it impossible to keep existing connections open indefinitely (which is a use case that should be supported), whereas force-closing can easily be implemented on top of the current behavior - just maintain a list of open connections. There is probably already a npm module for that.

[kanongil]

force-closing can easily be implemented on top of the current behavior - just maintain a list of open connections.

While tracking the connections is somewhat simple, actually determining the state of these, and acting on it, is not. As far as I am concerned, a graceful server close should:

1. Stop accepting new connections (existing behavior).
2. Immediately close any completely idle connections. Eg. connections with no incoming our outgoing messages pending.
3. Stop accepting new requests on all remaining connections, and closing these once the outgoing queue is drained.

For 2., the action is simple (a close on the socket) but detecting when to apply it seems tricky.

The major pain point is 3., which I don't think is possible to solve using public APIs, and maybe not even using private ones, as some of the state is captured in a closure scope.