Improve drainServer for Fastify

[glasser]

In #5635 (probably AS3.2) we add the drainServer hook, a built-in ApolloServerPluginDrainHttpServer plugin that works on Node http.Servers, and a Hapi-specific server.stop() drain plugin.

Figuring out exactly what to do with Fastify seems a bit subtle. Fastify provides an app.close function which invokes some sort of complex onClose system. Among other things, it will call close on the http server and block for that to return (which means that all connections must finish).

We provide sample code on https://www.apollographql.com/docs/apollo-server/integrations/middleware/#apollo-server-fastify (and in the apollo-server-fastify ApolloServer.test.ts) which installs a custom plugin that calls app.close alongside the standard ApolloServerPluginDrainHttpServer plugin. This is a bit janky because both plugins are going to try to close the http server, and so the second one to do so will get an error! It's OK-ish if ApolloServerPluginDrainHttpServer gets that error because it ignores errors from close, but if the closes get called in the other order maybe it's bad?

The problem is that if you don't include ApolloServerPluginDrainHttpServer then nothing actively closes idle connections and you'll end up waiting until your process is forcibly killed. But if you don't call app.close then I guess other parts of the Fastify lifecycle might not run?

The ideal solution might be to replace the http server inside Fastify with one with a fancier app.close. Or maybe it's to use a version/option of ApolloServerPluginDrainHttpServer that doesn't actually call close itself, just tries to deal with open connections?

Figuring out the details here probably involves a lot more Fastify expertise than exists on the Apollo Server team. So for now we're going to not add any sort of app.close plugin to the apollo-server-fastify API, recommend some kinda verbose boilerplate in the docs that links here, and hope somebody more excited about Fastify submits a better answer.

[mannyistyping]

Hey there! 👋🏽

I wanted to provide some updates that I believe apply to this issue/topic

Issue #3617 was raised by @jsumners discussing draining connections on close.

The issue was closed with a PR #3169 referred to as a "hacky solution" until the issue could be handled further down within Node itself.

The topic was addressed by @ShogunPanda in Node with PR #41578 which added to http.server

• closeAllConnections
• closeIdleConnections

What does that mean at this point?

referencing #5635

This was great... but only apollo-server worked this way, because only
apollo-server has full knowledge and control over its HTTP server.
This PR adds a server draining step to the ApolloServer lifecycle and
plugin interface, and provides a built-in plugin which drains a Node
http.Server using the logic of the first three steps above.

#41578 comment

"... have solved this same problem by exposing a new method that handles the closing of requests & connections without the user having to understand the intricacies underneath...Go solved this by adding a .Close method to their http module. This is an alternative to their existing .Shutdown method which works as Node's .close method currently does..."

Referencing Go's .close method:

"...Close immediately closes all active net.Listeners and any connections in state StateNew, StateActive, or StateIdle. For a graceful shutdown, use Shutdown...."

If I am understanding the conversations and implementations that took place, at the Node-level there are now ways to not only gracefully shutdown but to also forcibly close all connections and prevent further requests to be made and also idle connections from "hanging out"

At the Fastify level there now exists means to forcibly close all connections, but I'd imagine that with the recent landings to Node, there should be the ability to close both active and idle connections.

Maybe this could relate to enhancing the general plugin mentioned in #5635?
Maybe that means there can be an update to the Fastify Drain Server Example?
What do y'all think?

At a minimum, I wanted to provide some updates here to recent happenings!

Stay frosty,

• Manny
  (🤫, @MannyPamintuanAtCare, @MannyIsTypingOnAComputer)

[glasser]

Interesting!

Note that using these new http.Server methods requires using Node v18.2, which is higher than the minimum requirement for Apollo Server (and even the currently planned minimum requirement for AS4). But since in AS4 the Fastify integration will be a separately maintained and versioned package, it would be fine for it to have stricter Node version requirements than AS proper.

[trevor-scheer]

Possibly interesting to @olyop over in the https://github.com/apollo-server-integrations/apollo-server-integration-fastify repo.