Draining connections on close

[jsumners]

Given the simple service:

'use strict'

const fastify = require('fastify')({
  logger: { level: 'debug' },
  keepAliveTimeout: 60000
})

process.on('SIGINT', () => {
  fastify.close(() => {
    process.exit()
  })
})

fastify.route({
  path: '/',
  method: 'GET',
  handler (req, res) {
    res.send({ hello: 'world' })
  }
})

fastify.listen(8000)

We can start the server and then issue the following request to it:

GET / HTTP/1.1
connection: keep-alive
keep-alive: timeout=30, max=100

Subsequently, we send SIGINT to the server to indicate we want it to shutdown. At this point, we will see that the server does not shutdown before the keep-alive has completed. Our tests show that new connections are not possible, and re-usage of the existing connection should be met with a 503: https://github.com/fastify/fastify/blob/93fe532986b96ae28a087b2ec385a8abb16cce55/test/close.test.js

Video showing server wait

fastify-shutdown.mp4

What we do not seem to have is a forceful termination of those open connections. I recognize that the spec says:

A host may keep an idle connection open for longer than timeout seconds, but the host should attempt to retain a connection for at least timeout seconds.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Keep-Alive

But when we are shutting down, we usually don't want to do that.

I think our fastify.close method is directly mapped to the avvio close method. If we want to support forceful connection draining, would we do it here?:

fastify/lib/server.js

Lines 177 to 179 in 93fe532

	function close () {
	this.close()
	}

[mcollina]

I have researched this in great detail and it is at odds with one of our goals, performance.

The problem is that in order to close immediately we need to track the state of all sockets, e.g. adding them on a list when they are idle/keepalive and remove them when they are active. This was unfortunately slow a few years back.

I think we would have much more success in implementing this in Node itself in C++, but a check if this could be done without significant overhead here would be awesome.

[jsumners]

Something must be keeping track of theses connections in order to reuse them.

[jsumners]

I am really confused about what fastifyInstance.close() actually does to initiate shutdown. Stepping through it takes me through avvio methods .close, .boot, .ready, and .start. I never see where we actually do fastifyInstance.server.close().

[climba03003]

I am really confused about what fastifyInstance.close() actually does to initiate shutdown. Stepping through it takes me through avvio methods .close, .boot, .ready, and .start. I never see where we actually do fastifyInstance.server.close().

we push to the onClose hook and close the server inside there.

fastify/fastify.js

Lines 369 to 381 in 93fe532

	// cache the closing value, since we are checking it in an hot path
	avvio.once('preReady', () => {
	fastify.onClose((instance, done) => {
	fastify[kState].closing = true
	router.closeRoutes()
	if (fastify[kState].listening) {
	// No new TCP connections are accepted
	instance.server.close(done)
	} else {
	done(null)
	}
	})
	})

when it call close it execute the below function first.
https://github.com/fastify/avvio/blob/d9a9bbf5467488a00f0ec7a4edfcbb7ce3f8ea17/boot.js#L70-L82

then execute
https://github.com/fastify/avvio/blob/d9a9bbf5467488a00f0ec7a4edfcbb7ce3f8ea17/boot.js#L304-L329

[jsumners]

Thanks. That makes the waters a little less murky. Unfortunately, adding a instance.server.unref() in there does not alleviate the root problem :(

[mcollina]

Something must be keeping track of theses connections in order to reuse them.

Nothing. Some more bytes pops up on the socket, those are parsed by Node's HTTP parser, this is then translated to a request event being emitted on the server instance.
The sockets have a reference to the server, not vice versa.

[jsumners]

That really complicates being able to clean up a process on shutdown.

[mcollina]

As far as I recall adding that tracking cost 30% of throughput to Hapi back in the days

[jsumners]

Resolved by #3619

[bompus]

I'm putting my example, which I've been using prior to this feature being available, here for discussion, since the new forceCloseConnections option does not handle in-progress connections.

for (const socket of socketsSet) {
   const serverResponse = socket._httpMessage;
   if (serverResponse?.headersSent === false) {
     // this socket is an active request, so give it some time to finish naturally
     serverResponse.setHeader('connection', 'close');
     continue;
   }

   // this is likely a keep-alive socket that is not part of an active request
   socket.destroy();
   socketsSet.delete(socket);
}

but then of course, I have to add some sanity to my graceful shutdown process so a client connection can't keep it from closing down forever...

setTimeout(() => {
   throw new Error('gracefulStopAsync timed out');
}, 10000).unref();

[jsumners]

Thank you. But two things:

1. We will not rely on internals for a supported feature
2. That check seems to only be good for outbound requests

[bompus]

Thanks for the feedback. It seemed to work for my use case, but I wrote that a year ago and haven't looked at it since.

I was mainly just trying to start some discussion on how to use the newly supported feature alongside some type of detection for in-progress requests to allow some time for them to gracefully close before forcefully closing them. Reviewing the PR, the way the new feature is written doesn't appear that it can be hooked into any way, so I was trying to figure out what to do - use my method of tracking - or the core feature and live with sometimes forcefully closing in-progress requests.

[jsumners]

Please subscribe to nodejs/node#41578

If that feature gets implemented, we would deprecate this feature in Fastify and replace it with direct core support. The implementation in core should consider if connections have active requests and allow them to finish before terminating the connection.

As noted in the docs with this feature in Fastify, it is a very forceful close. Use with care.