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

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

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

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.

@kanongil

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

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

@tshemsedinov

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

@tshemsedinov

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.

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

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.

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.

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.

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.

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.

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:

Stop accepting new connections (existing behavior).
Immediately close any completely idle connections. Eg. connections with no incoming our outgoing messages pending.
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.

If anyone is interested, I coded up a function that will enable graceful closing on an http server. Heads up that it's written in TypeScript, but it shouldn't be too hard to understand. Hopefully this is helpful to others that have been wrestling with this.

https://gist.github.com/jinxidoru/0611100d1d12ecddfa04

rip out the special handeling that is no longer needed because nodejs…

stoppable seems very inactive and not enough for me, also has issues without reply, so I ended with:
https://github.com/LuKks/like-server
Would be great if we can test it more.

Destroy keep alive connections on server close

fix: Properly handle keep-alive connections

Resolve tests timeout and closes connections

Resolve tests timeout and closes connections (#1755)

This is insane.. Also spent 8 hours trying to understand why a server close would take 5 seconds, well that's because of this because the keepAliveTimeout defaults to 5000

Even server.closeAllConnections(); doesn't force close them, maybe this needs a force argument, otherwise this really shouldn't be a closed issue

@Tofandel

@Tofandel Sounds implausible, it basically calls socket.destroy() on all connections it knows about. Please put together a small test case and open a new issue if you're sure that is what's happening.

Consider this module: https://github.com/LuKks/graceful-http

I based the library on this (plus on real usage):
https://blog.dashlane.com/implementing-nodejs-http-graceful-shutdown/
That blog post explains the problem and solution perfectly!

My simple solution is to destroy sockets on request finish:
server.on('request', (request, response) => {
	response.on('finish', () => request.socket.destroy());
});
This way, when you call server.close() only real in-progress requests are considered before closing.

I am on node.js 16.15.0 and by using your suggestion, able to invoke close callback properly

server.close((err) => {
  if (err) {
    console.error(err);
  }
  
  console.info("http server closed successfully. Exiting!");
});

best part is I don't have to use process.exit(0) so other processes continue working and won't cluttered by explicit process.exit

Navigation Menu

Repository navigation

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

27 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Activity

brendanashworth commented on Sep 2, 2015

kanongil commented on Sep 2, 2015

tshemsedinov commented on Sep 5, 2015

ronkorving commented on Sep 5, 2015

kanongil commented on Sep 5, 2015

codygustafson commented on Sep 7, 2015

ronkorving commented on Sep 7, 2015

codygustafson commented on Sep 7, 2015

tpetry commented on Oct 4, 2015

bnoordhuis commented on Oct 4, 2015

jinxidoru commented on Oct 4, 2015

bnoordhuis commented on Oct 4, 2015

kanongil commented on Oct 4, 2015

jinxidoru commented on Oct 6, 2015

27 remaining items

LuKks commented on Jul 16, 2019

Tofandel commented on Jan 21, 2023

bnoordhuis commented on Jan 22, 2023

LuKks commented on Jan 22, 2023

Kjaer commented on Feb 13, 2023

four43 commented on Feb 13, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions