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

@@ -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)

docs/source/api/apollo-server.md

@@ -338,7 +338,7 @@ In certain cases, Apollo Server installs some of its built-in plugins automatica
 </td>
 <td>
 
-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`.
 

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<void>>();
+  private toDisposeLast = new Set<() => Promise<void>>();
   private experimental_approximateDocumentStoreMiB:
     Config['experimental_approximateDocumentStoreMiB'];
 
@@ -356,15 +357,34 @@ 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`);
+            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.once(signal, handler);
-        this.toDispose.add(async () => {
+        process.on(signal, handler);
+        this.toDisposeLast.add(async () => {
           process.removeListener(signal, handler);
         });
       });
@@ -596,8 +616,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) {