From 3046b8a7efbd209de0729e61cb84bb1753220919 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Fri, 5 Mar 2021 17:08:53 -0800 Subject: [PATCH] apollo-server-core: Improve SIGINT/SIGTERM handling Two improvements to the SIGINT and SIGTERM signal handlers that are installed by default in non-test mode if you don't pass `stopOnTerminationSignals: false`. - If another SIGINT or SIGTERM is recieved while awaiting `stop()`, ignore it (ie, prevent the process from exiting if this was the only handler for that signal). This is implemented by switching the `process.once` to `process.on` and explicitly tracking if a signal has been received. Fixes #4931. - If `await this.stop()` throws, make sure to shut down the process anyway after logging. This does `process.exit(1)` instead of re-signaling because it can't be sure that the signal handler has been removed. --- CHANGELOG.md | 1 + docs/source/api/apollo-server.md | 2 +- .../apollo-server-core/src/ApolloServer.ts | 30 +++++++++++++++++-- 3 files changed, 29 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c94e5e413cd..559292af5f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ The version headers in this history reflect the versions of Apollo Server itself > The changes noted within this `vNEXT` section have not been released yet. New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development. With few exceptions, the format of the entry should follow convention (i.e., prefix with package name, use markdown `backtick formatting` for package names and code, suffix with a link to the change-set à la `[PR #YYY](https://link/pull/YYY)`, etc.). When a release is being prepared, a new header will be (manually) created below and the appropriate changes within that release will be moved into the new section. +- `apollo-server-core`: The `SIGINT` and `SIGTERM` signal handlers installed by default (when not disabled by `stopOnTerminationSignals: false`) now stay active (preventing process termination) while the server shuts down, instead of letting a second signal terminate the process. The handlers still re-signal the process after `this.stop()` concludes. Also, if `this.stop()` throws, the signal handlers will now log and exit 1 instead of throwing an uncaught exception. [Issue #4931](https://github.com/apollographql/apollo-server/issues/4931) - `apollo-server-lambda`: The `onHealthCheck` option did not previously work. Additionally, health checks (with `onHealthCheck` or without) didn't work in all Lambda contexts, such as behind Custom Domains; the path check is now more flexible. [Issue #3999](https://github.com/apollographql/apollo-server/issues/3999) [PR #4969](https://github.com/apollographql/apollo-server/pull/4969) [Issue #4891](https://github.com/apollographql/apollo-server/issues/4891) [PR #4892](https://github.com/apollographql/apollo-server/pull/4892) - The `debug` option to `new ApolloServer` (which adds stack traces to errors) now affects errors that come from requests executed with `server.executeOperation` (and its wrapper `apollo-server-testing`), instead of just errors that come from requests executed over HTTP. [Issue #4107](https://github.com/apollographql/apollo-server/issues/4107) [PR #4948](https://github.com/apollographql/apollo-server/pull/4948) - Bump version of `@apollographql/graphql-playground-html` to v1.6.27 and `@apollographql/graphql-playground-react` to v1.7.39 to resolve incorrectly rendered CDN URL when Playground `version` was `false`-y. [PR #4932](https://github.com/apollographql/apollo-server/pull/4932) [PR #4955](https://github.com/apollographql/apollo-server/pull/4955) [Issue #4937](https://github.com/apollographql/apollo-server/issues/4937) diff --git a/docs/source/api/apollo-server.md b/docs/source/api/apollo-server.md index 95b8ea4c1cd..41f580e8033 100644 --- a/docs/source/api/apollo-server.md +++ b/docs/source/api/apollo-server.md @@ -338,7 +338,7 @@ 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. It then sends that same signal to itself to continue process shutdown. +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. Set this option to `false` to disable this default behavior, or to `true` to enable the behavior even when `NODE_ENV` _does_ equal `test`. diff --git a/packages/apollo-server-core/src/ApolloServer.ts b/packages/apollo-server-core/src/ApolloServer.ts index 49ae83deb6f..af017f8cd25 100644 --- a/packages/apollo-server-core/src/ApolloServer.ts +++ b/packages/apollo-server-core/src/ApolloServer.ts @@ -149,6 +149,7 @@ export class ApolloServerBase { /** @deprecated: This is undefined for servers operating as gateways, and will be removed in a future release **/ protected schema?: GraphQLSchema; private toDispose = new Set<() => Promise>(); + private toDisposeLast = new Set<() => Promise>(); private experimental_approximateDocumentStoreMiB: Config['experimental_approximateDocumentStoreMiB']; @@ -356,15 +357,33 @@ export class ApolloServerBase { : isNodeLike && process.env.NODE_ENV !== '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 () => { - await this.stop(); + 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: ${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.once(signal, handler); - this.toDispose.add(async () => { + process.on(signal, handler); + this.toDisposeLast.add(async () => { process.removeListener(signal, handler); }); }); @@ -596,8 +615,13 @@ export class ApolloServerBase { } public async stop() { + // We run shutdown handlers in two phases because we don't want to turn + // off our signal listeners until we've done the important parts of shutdown + // like running serverWillStop handlers. (We can make this more generic later + // if it's helpful.) await Promise.all([...this.toDispose].map(dispose => dispose())); if (this.subscriptionServer) this.subscriptionServer.close(); + await Promise.all([...this.toDisposeLast].map(dispose => dispose())); } public installSubscriptionHandlers(server: HttpServer | HttpsServer | Http2Server | Http2SecureServer | WebSocket.Server) {