No great pattern for doing a graceful shutdown with Apollo Server integration packages

[vieira]

Hello,

After updating to 2.22 (see PR #4981) and applying the recommendation in changelog to insert await server.start() between server = new ApolloServer() and server.applyMiddleware we started observing that Apollo is now listetning to termination signals and stops handling in-flight requests by throwing:

{"errors": [{
  "message": "Cannot execute GraphQL operations after the server has stopped.",
  "extensions": {"code":"INTERNAL_SERVER_ERROR"}
}]}

We were already handling these signals and calling the express close method (which does not abort in-flight requests but rather stops accepting new ones and waits for the others to finish).

My impression was that when using some middleware, like express, rather than the standalone apollo server these signals should not be handled by apollo itself? At least they were not prior to 2.22.x.

To work around this issue we explicitely set stopOnTerminationSignals: false and it seems to have resolved it.

Some context: We are deploying to a K8S deployment which does a rolling update. After the new version has started k8s sends a termination signal to the old version. Upon receiving this signal we make the readiness probe fail to avoid new requests being routed but keep express up for some more time until the in-flight requests are finished (or a timeout is triggered).

[glasser]

Yes, if you're using apollo-server-express I'd suggest you use stopOnTerminationSignals: false and handle signals yourself. It would be best to completely stop listening to requests at the http level before calling server.stop().

If you're using apollo-server (the package which bundles its own express), it in fact does that automatically for you since AS v2.20. (We use the stoppable npm package instead of just httpServer.close, which closes idle HTTP connections and eventually force-closes all connections after a grace period instead of just waiting for all connections to be naturally closed.)

The only difference in v2.22 is that we're more strict about not letting you run operations during or after the server has stopped.

I think the main "bug" here is that the default signal handling behavior doesn't give you much of a hook for doing the right thing yourself. Using stopOnTerminationSignals: false and expecting users to set up their own handlers isn't the worst solution, but it would be nice if there was an easy way to stick with Apollo Server's signal handlers and have them close the server for you. For example, there could be a method that lets you register an HTTP server with Apollo Server, and then move the apollo-server-specific stoppable code into apollo-server-core.

[glasser]

I think I want to add a method to ApolloServer in apollo-server-core like ApolloServer.registerHttpServerForStop({httpServer: http.Server, stopGracePeriodMillis?: number}). I'm not sure I like that particular name, though.

[glasser]

Maybe we just need a "pre stop handler" (and have a phase before "stopping" that's like "stopping" but operations still run). This will also give an appropriate place to shut down subscriptions servers.

[glasser]

I think we should fork stoppable either into @apollographql/stoppable or just be a function exported from apollo-server-core which (a) fixes hunterloftis/stoppable#33 / #4933 and (b) provides a Promise-based version of stop. Then we can add a built-in plugin that applies our stoppable to an http server and shuts it down at pre-stop time. (Maybe don't even need to export the function, just use it in the plugin.l)

[glasser]

OK, I think this is my full proposal.

• Add an async drainServer() plugin hook on GraphQLServerListener (with docs, etc).
• Add a draining phase to ServerState. This state should generally function the same as started (including having schemaDerivedData on it). This is a nice opportunity to find references on phase and see what it controls. Notably, operations should run during this phase! The only real difference from started should be that if you call stop() in the draining phase, it waits for the stop() in progress to end rather than beginning shutdown itself.
• ApolloServer.stop() (in apollo-server-core) should transition into draining before stopping and invoke the drainServer hooks. Make sure all the logic around "if stop is called when some other thread is already stopping it, it just waits for the other thread to be done" still works.
• Add a new built-in plugin (with docs etc), ApolloServerPluginDrainHTTPServer. This plugin takes {httpServer: http.Server, stopGracePeriodMillis?: number}. It wraps the given server with stoppable, and it drainServer() by calling stop() on it.
• Maybe fork stoppable into apollo-server-core (not exported) to fix some issues with it (Avoid using timer.unref hunterloftis/stoppable#33, probably don't actually need to put the stop() call on the server, don't need to write a _pendingSockets field that's never read, etc). I might just do this myself now.
• Change docs for apollo-server-express etc as described below
• Update stop docs.
• Update apollo-server so that instead of overriding stop(), it adds ApolloServerPluginDrainHTTPServer to the given plugins. Note that this will require creating the app and httpServer in its constructor (but we don't have to actually add behavior to app until listen().
• Update subscription docs to show how to use the drainServer hook to close the subscription server instead of signal handling.

Re docs, we'll need to change from:

async function startApolloServer(typeDefs, resolvers) {
  const server = new ApolloServer({ typeDefs, resolvers });
  await server.start();
  const app = express();
  server.applyMiddleware({ app });
  await new Promise(resolve => app.listen({ port: 4000 }, resolve));
  console.log(`🚀 Server ready at http://localhost:4000${server.graphqlPath}`);
}

to

async function startApolloServer(typeDefs, resolvers) {
  const app = express();
  const httpServer = http.createServer(app);
  const server = new ApolloServer({ typeDefs, resolvers, plugins: [ApolloServerPluginDrainServer(httpServer)] });
  await server.start();
  server.applyMiddleware({ app });
  await new Promise(resolve => httpServer.listen({ port: 4000 }, resolve));
  console.log(`🚀 Server ready at http://localhost:4000${server.graphqlPath}`);
}

[glasser]

(starting on the inline-and-improve-stoppable project at #5498 )

[s123121]

Sorry to chime in here, but I came here from the docs https://www.apollographql.com/docs/apollo-server/migration/

I am in the process of migrating Apollo Server from 2.16 to 3.0.2, with the SIGINT workaround

['SIGINT', 'SIGTERM'].forEach(signal => {
    process.on(signal, () => subscriptionServer.close());
  });

My dev machine (Mac M1) start giving error as follow:

Error: read EIO
   at TTY.onStreamRead (node:internal/stream_base_commons:211:20)
Emitted 'error' event on ReadStream instance at:
   at emitErrorNT (node:internal/streams/destroy:188:8)
   at emitErrorCloseNT (node:internal/streams/destroy:153:3)
   at processTicksAndRejections (node:internal/process/task_queues:81:21) {
 errno: -5,
 code: 'EIO',
 syscall: 'read'
}

Remove the shut down code make everything work again. Is this related? Do we really need the above code?

[glasser]

You're welcome to not shut down your server cleanly if you find that works for you. We're planning to work soon on fixing this issue to give a way to put the shutdown at an appropriate time in the server lifecycle.

[kevin-lindsay-1]

I would like to throw this out there just so that it's acknowledged: right now it looks like stop also shuts down the health check endpoint, which can then cause readinessProbes to fail in kubernetes, which then prevents the request from completing.

I haven't looked into the code, so maybe I'm missing something, but it seems like that's the case, and if so it would be good to keep in mind that in kubernetes the health check should stay active until the pod is ready to be removed. At least, that's what I intuit, as once a pod enters terminating state it's generally considered finished cleaning up once it stops being ready.

Something else could be going on; I'll follow up after I figure out what's up with this.

---

Edit: Upon further examination, the issue was caused by Istio needing a pod annotation of:

# https://istio.io/latest/docs/reference/config/istio.mesh.v1alpha1/#ProxyConfig
proxy.istio.io/config: |
  terminationDrainDuration: {{ $terminationGracePeriodSeconds }}s

[trevor-scheer]

@glasser jut as a heads up, it looks like this issue is now affecting us, or we've begun to observe/address it recently.
cc @AlexanderMann

[glasser]

@kevin-lindsay-1 hmm, are you combining liveness and readiness probes here? readiness is about routing, liveness is about "should restart". And do liveness probes actually continue to be relevant once you're already shutting down?

[kevin-lindsay-1]

@glasser my liveness and readiness probes both use /.well-known/.../health as the endpoint; if it stops responding momentarily, it's not ready, and requests are load balanced to other pods in the deployment. If it stops responding for a long time, it's presumed to be dead, which has never happened to my services to my knowledge.

A pod will also stop when the process exits, however I'm not sure if a successful apolloServer.stop() causes process.exit(0) to be called. If a pod stops while it's not being terminated, it is by default restarted with a crash-loop backoff.

Once you're in the terminating state on a pod, I believe that it stops routing requests and expects the process to exit within terminationGracePeriodSeconds (default 30s), so I don't think liveness probes are checked for that.