Skip to content

Commit

Permalink
apollo-server-core: register signal handlers later and not on serverl…
Browse files Browse the repository at this point in the history 
…ess (#5639)

`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.
  • Loading branch information
@glasser
glasser authored Aug 20, 2021
1 parent 50c993b commit 038bee9
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 43 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
6 changes: 4 additions & 2 deletions docs/source/api/apollo-server.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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.

Expand Down
89 changes: 48 additions & 41 deletions packages/apollo-server-core/src/ApolloServer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand All @@ -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
Expand Down

0 comments on commit 038bee9

Please sign in to comment.