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.

Author: glasser

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 #FIXME](https://github.com/apollographql/apollo-server/pull/FIXME)
 
 ## v3.1.2
 

docs/source/api/apollo-server.md

@@ -289,10 +289,12 @@ 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. (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.
 
 </td>
@@ -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.
 

packages/apollo-server-core/src/ApolloServer.ts

@@ -122,6 +122,7 @@ export class ApolloServerBase<
   private toDispose = new Set<() => Promise<void>>();
   private toDisposeLast = new Set<() => Promise<void>>();
   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