Default behaviour of GraphQLModule.stopOnTerminationSignals doesn't interact nicely with Nest enableShutdownHooks

ssarj commented 3 years ago

I'm submitting a...


[ ] Regression 
[x] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

Referenced repository contains a barebones Nest application that has:

A single service that implements the Nest lifecycle events OnModuleDestroy and OnApplicationShutdown and logs when they're called (shutdown hooks are enabled)
A single GraphQL resolver / model

If I start the server and then kill it (either via CTRL+C, or sending SIGTERM) then Service.onModuleDestroy is called, but Service.onApplicationShutdown never is. This appears to be due to the default behaviour of GraphQLModule / apollo-server-express which also has Apollo listening to process signals. It then kills the process before Nest can shutdown gracefully.

Expected behavior

That all lifecycle events are executed, and Service.onApplicationShutdown is called when shutdown hooks have been enabled in the Nest application, and the user isn't required to know that they have to turn off Apollo signal handling.

Minimal reproduction of the problem with instructions

Repository
main.ts. NOTE: this has stopOnTerminationSignals set to false so executes the desired behaviour

What is the motivation / use case for changing the behavior?

Environment


  "@apollo/gateway": "^0.26.3",
  "@nestjs/common": "^7.6.15",
  "@nestjs/core": "^7.6.15",
  "@nestjs/graphql": "^7.10.6",
  "@nestjs/platform-express": "^7.6.15",
  "apollo-server-express": "^2.24.1",
  "graphql": "^15.5.0",
  "graphql-tools": "^7.0.5",


For Tooling issues:
- Node version: 16.1.0
- Platform:  Linux (Ubuntu) under wsl

Others:

kamilmysliwiec commented 3 years ago

Would you like to create a PR for this issue? The only suitable solution that I can see for now would be to set stopOnTerminationSignals to false by default.

ssarj commented 3 years ago

Yep, will do. Thanks

kamilmysliwiec commented 3 years ago

Let's track this here https://github.com/nestjs/graphql/pull/1562

mareksuscak commented 1 month ago

@kamilmysliwiec this issue needs to be re-opened as this is not working again (or still) when nestjs/graphql is paired with platform-fastify.

EDIT: I think this is broken again after the drain server plugin was set up globally because the drain plugin calls fastify.close() - see this

EDIT 2: I think we should drop the drain plugins and implement connection draining in Nest in such a way that it gracefully shuts down the Express or Fastify server. Here's how Apollo Express Driver does it: https://github.com/apollographql/apollo-server/blob/main/packages/server/src/plugin/drainHttpServer/stoppable.ts

nestjs / graphql