From ef6447b7eb632495e24a88c3e52c63f443d5cd53 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Fri, 20 Aug 2021 12:05:00 -0700 Subject: [PATCH] apollo-server-core: register signal handlers later and not on serverless `stop()` is not designed to work properly if `start()` has not previously succeeded (there's an XXX comment about this), and #5635 is going to make early `stop()` calls into a hard error. This change ensures that the signal handler won't cause that error to show up. Also, serverless integrations don't use the same sort of process-based shutdown as other environments (and you don't call `start` or `listen` yourself), so registering these signal handlers isn't a great default. (They start "listening" before the ApolloServer is started, so it would be weird if after this change the signal handling depended on whether or not a request had been processed or not.) Make stopOnTerminationSignals default to false for serverless integrations. --- CHANGELOG.md | 1 + docs/source/api/apollo-server.md | 6 +- .../apollo-server-core/src/ApolloServer.ts | 89 ++++++++++--------- 3 files changed, 53 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e47e4937cdb..91fe9f95803 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ The version headers in this history reflect the versions of Apollo Server itself ## vNEXT - `apollo-server-core`: Fix `experimental_approximateDocumentStoreMiB` option, which seems to have never worked before. [PR #5629](https://github.com/apollographql/apollo-server/pull/5629) +- `apollo-server-core`: Only register `SIGINT` and `SIGTERM` handlers once the server successfully starts up; trying to call `stop` on a server that hasn't successfully started had undefined behavior. By default, don't register the handlers in serverless integrations, which don't have the same lifecycle as non-serverless integrations (eg, there's no explicit `start` call); you can still explicitly set `stopOnTerminationSignals` to override this default. [PR #5639](https://github.com/apollographql/apollo-server/pull/5639) ## v3.1.2 diff --git a/docs/source/api/apollo-server.md b/docs/source/api/apollo-server.md index a67db2fadb9..d687dbbb747 100644 --- a/docs/source/api/apollo-server.md +++ b/docs/source/api/apollo-server.md @@ -289,10 +289,12 @@ In certain cases, Apollo Server installs some of its built-in plugins automatica -By default (when running in Node and when the `NODE_ENV` environment variable does not equal `test`), whenever Apollo Server receives a `SIGINT` or `SIGTERM` signal, it calls `await this.stop()` on itself. (While this call to `this.stop()` is running, it ignores all `SIGINT` and `SIGTERM` signals.) It then sends that same signal to itself to continue process shutdown. +By default (when running in Node when the `NODE_ENV` environment variable does not equal `test` and not using a [serverless-specific package](../integrations/middleware/#all-supported-packages)), whenever Apollo Server receives a `SIGINT` or `SIGTERM` signal, it calls `await this.stop()` on itself. (While this call to `this.stop()` is running, it ignores all `SIGINT` and `SIGTERM` signals.) It then sends that same signal to itself to continue process shutdown. Set this option to `false` to disable this default behavior, or to `true` to enable the behavior even when `NODE_ENV` _does_ equal `test`. +The signal handler is installed after [`start()`](#start) returns successfully. + You can also manually call `stop()` in other contexts. Note that `stop()` is asynchronous, so it isn't useful in a `process.on('exit')` handler. @@ -515,7 +517,7 @@ The `start` method triggers the following actions: ## `stop` -`ApolloServer.stop()` is an async method that tells all of Apollo Server's background tasks to complete. It calls and awaits all [`serverWillStop` plugin handlers](../integrations/plugins-event-reference/#serverwillstop) (including the [usage reporting plugin](./plugin/usage-reporting/)'s handler, which sends a final usage report to Apollo Studio). This method takes no arguments. +`ApolloServer.stop()` is an async method that tells all of Apollo Server's background tasks to complete. It calls and awaits all [`serverWillStop` plugin handlers](../integrations/plugins-event-reference/#serverwillstop) (including the [usage reporting plugin](./plugin/usage-reporting/)'s handler, which sends a final usage report to Apollo Studio). This method takes no arguments. You should only call it after [`start()`](#start) returns successfully (or [`listen()`](#listen) if you're using the batteries-included `apollo-server` package). If your server is a [federated gateway](https://www.apollographql.com/docs/federation/gateway/), `stop` also stops gateway-specific background activities, such as polling for updated service configuration. diff --git a/packages/apollo-server-core/src/ApolloServer.ts b/packages/apollo-server-core/src/ApolloServer.ts index 64e3795b786..2978154b5a3 100644 --- a/packages/apollo-server-core/src/ApolloServer.ts +++ b/packages/apollo-server-core/src/ApolloServer.ts @@ -122,6 +122,7 @@ export class ApolloServerBase< private toDispose = new Set<() => Promise>(); private toDisposeLast = new Set<() => Promise>(); private experimental_approximateDocumentStoreMiB: Config['experimental_approximateDocumentStoreMiB']; + private stopOnTerminationSignals: boolean; private landingPage: LandingPage | null = null; // The constructor should be universal across all environments. All environment specific behavior should be set by adding or overriding methods @@ -191,6 +192,15 @@ export class ApolloServerBase< : process.env.NODE_ENV; const isDev = nodeEnv !== 'production'; + // We handle signals if it was explicitly requested, or if we're in Node, + // not in a test, not in a serverless framework, and it wasn't explicitly + // turned off. (We only actually register the signal handlers once we've + // successfully started up, because there's nothing to stop otherwise.) + this.stopOnTerminationSignals = + typeof stopOnTerminationSignals === 'boolean' + ? stopOnTerminationSignals + : isNodeLike && nodeEnv !== 'test' && !this.serverlessFramework(); + // if this is local dev, introspection should turned on // in production, we can manually turn introspection on by passing { // introspection: true } to the constructor of ApolloServer @@ -227,47 +237,6 @@ export class ApolloServerBase< // is populated accordingly. this.ensurePluginInstantiation(plugins, isDev); - // We handle signals if it was explicitly requested, or if we're in Node, - // not in a test, and it wasn't explicitly turned off. - if ( - typeof stopOnTerminationSignals === 'boolean' - ? stopOnTerminationSignals - : isNodeLike && nodeEnv !== 'test' - ) { - const signals: NodeJS.Signals[] = ['SIGINT', 'SIGTERM']; - let receivedSignal = false; - signals.forEach((signal) => { - // Note: Node only started sending signal names to signal events with - // Node v10 so we can't use that feature here. - const handler: NodeJS.SignalsListener = async () => { - if (receivedSignal) { - // If we receive another SIGINT or SIGTERM while we're waiting - // for the server to stop, just ignore it. - return; - } - receivedSignal = true; - try { - await this.stop(); - } catch (e) { - this.logger.error(`stop() threw during ${signal} shutdown`); - this.logger.error(e); - // Can't rely on the signal handlers being removed. - process.exit(1); - } - // Note: this.stop will call the toDisposeLast handlers below, so at - // this point this handler will have been removed and we can re-kill - // ourself to die with the appropriate signal exit status. this.stop - // takes care to call toDisposeLast last, so the signal handler isn't - // removed until after the rest of shutdown happens. - process.kill(process.pid, signal); - }; - process.on(signal, handler); - this.toDisposeLast.add(async () => { - process.removeListener(signal, handler); - }); - }); - } - if (gateway) { // ApolloServer has been initialized but we have not yet tried to load the // schema from the gateway. That will wait until the user calls @@ -483,6 +452,7 @@ export class ApolloServerBase< phase: 'started', schemaManager, }; + this.maybeRegisterTerminationSignalHandlers(['SIGINT', 'SIGTERM']); } catch (error) { this.state = { phase: 'failed to start', error }; throw error; @@ -491,6 +461,43 @@ export class ApolloServerBase< } } + private maybeRegisterTerminationSignalHandlers(signals: NodeJS.Signals[]) { + if (!this.stopOnTerminationSignals) { + return; + } + + let receivedSignal = false; + const signalHandler: NodeJS.SignalsListener = async (signal) => { + if (receivedSignal) { + // If we receive another SIGINT or SIGTERM while we're waiting + // for the server to stop, just ignore it. + return; + } + receivedSignal = true; + try { + await this.stop(); + } catch (e) { + this.logger.error(`stop() threw during ${signal} shutdown`); + this.logger.error(e); + // Can't rely on the signal handlers being removed. + process.exit(1); + } + // Note: this.stop will call the toDisposeLast handlers below, so at + // this point this handler will have been removed and we can re-kill + // ourself to die with the appropriate signal exit status. this.stop + // takes care to call toDisposeLast last, so the signal handler isn't + // removed until after the rest of shutdown happens. + process.kill(process.pid, signal); + }; + + signals.forEach((signal) => { + process.on(signal, signalHandler); + this.toDisposeLast.add(async () => { + process.removeListener(signal, signalHandler); + }); + }); + } + // This method is called at the beginning of each GraphQL request by // `graphQLServerOptions`. Most of its logic is only helpful for serverless // frameworks: unless you're in a serverless framework, you should have called