Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add serverWillStop lifecycle hook; call stop() on signals by default #4450

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions docs/source/api/apollo-server.md
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,17 @@ new ApolloServer({
size of 30MiB, which is generally sufficient unless the server is processing
a high number of unique operations.

* `stopOnTerminationSignals`: `boolean`

By default (when running in Node and when the `NODE_ENV` environment variable does not equal `test`),
ApolloServer listens for the `SIGINT` and `SIGTERM` signals and calls `await this.stop()` on
itself when it is received, and then re-sends the signal to itself so that process shutdown can continue.
Set this to false to disable this behavior, or to true to enable this behavior even when `NODE_ENV` is
`test`. You can manually invoke `stop()` in other contexts if you'd
like. Note that `stop()` does not run synchronously so it cannot work usefully in an `process.on('exit')`
handler.


#### Returns

`ApolloServer`
Expand Down Expand Up @@ -447,11 +458,8 @@ addMockFunctionsToSchema({

* `handleSignals`: boolean

By default, EngineReportingAgent listens for the 'SIGINT' and 'SIGTERM'
signals, stops, sends a final report, and re-sends the signal to
itself. Set this to false to disable. You can manually invoke 'stop()' and
'sendReport()' on other signals if you'd like. Note that 'sendReport()'
does not run synchronously so it cannot work usefully in an 'exit' handler.
For backwards compatibility only; specifying `new ApolloServer({engine: {handleSignals: false}})` is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a deprecation note similar to the ones immediately above? Most importantly, I'd just like to see the actual word "deprecated" here somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do (next week)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I'd rather this not merge until #4453 merges too, at which point this entire EngineReportingOptions section will be deprecated.

equivalent to specifying `new ApolloServer({stopOnTerminationSignals: false})`.

* `rewriteError`: (err: GraphQLError) => GraphQLError | null

Expand Down
31 changes: 3 additions & 28 deletions packages/apollo-engine-reporting/src/agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -290,11 +290,8 @@ export interface EngineReportingOptions<TContext> {
*/
privateHeaders?: Array<String> | boolean;
/**
* By default, EngineReportingAgent listens for the 'SIGINT' and 'SIGTERM'
* signals, stops, sends a final report, and re-sends the signal to
* itself. Set this to false to disable. You can manually invoke 'stop()' and
* 'sendReport()' on other signals if you'd like. Note that 'sendReport()'
* does not run synchronously so it cannot work usefully in an 'exit' handler.
* For backwards compatibility only; specifying `new ApolloServer({engine: {handleSignals: false}})` is
* equivalent to specifying `new ApolloServer({stopOnTerminationSignals: false})`.
*/
handleSignals?: boolean;
/**
Expand Down Expand Up @@ -445,8 +442,6 @@ export class EngineReportingAgent<TContext = any> {
private stopped: boolean = false;
private signatureCache: InMemoryLRUCache<string>;

private signalHandlers = new Map<NodeJS.Signals, NodeJS.SignalsListener>();

private currentSchemaReporter?: SchemaReporter;
private readonly bootId: string;
private lastSeenExecutableSchemaToId?: {
Expand Down Expand Up @@ -529,21 +524,6 @@ export class EngineReportingAgent<TContext = any> {
);
}

if (this.options.handleSignals !== false) {
const signals: NodeJS.Signals[] = ['SIGINT', 'SIGTERM'];
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 () => {
this.stop();
await this.sendAllReportsAndReportErrors();
process.kill(process.pid, signal);
};
process.once(signal, handler);
this.signalHandlers.set(signal, handler);
});
}

if (this.options.endpointUrl) {
this.logger.warn(
'[deprecated] The `endpointUrl` option within `engine` has been renamed to `tracesEndpointUrl`.',
Expand Down Expand Up @@ -847,11 +827,6 @@ export class EngineReportingAgent<TContext = any> {
// size, and stop buffering new traces. You may still manually send a last
// report by calling sendReport().
public stop() {
// Clean up signal handlers so they don't accrue indefinitely.
this.signalHandlers.forEach((handler, signal) => {
process.removeListener(signal, handler);
});

if (this.reportTimer) {
clearInterval(this.reportTimer);
this.reportTimer = undefined;
Expand Down Expand Up @@ -930,7 +905,7 @@ export class EngineReportingAgent<TContext = any> {
return generatedSignature;
}

private async sendAllReportsAndReportErrors(): Promise<void> {
public async sendAllReportsAndReportErrors(): Promise<void> {
await Promise.all(
Object.keys(this.reportDataByExecutableSchemaId).map(executableSchemaId =>
this.sendReportAndReportErrors(executableSchemaId),
Expand Down
60 changes: 49 additions & 11 deletions packages/apollo-server-core/src/ApolloServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import {
import {
ApolloServerPlugin,
GraphQLServiceContext,
GraphQLServerListener,
} from 'apollo-server-plugin-base';
import runtimeSupportsUploads from './utils/runtimeSupportsUploads';

Expand Down Expand Up @@ -76,13 +77,14 @@ import {
import { Headers } from 'apollo-server-env';
import { buildServiceDefinition } from '@apollographql/apollo-tools';
import { plugin as pluginTracing } from "apollo-tracing";
import { Logger, SchemaHash } from "apollo-server-types";
import { Logger, SchemaHash, ValueOrPromise } from "apollo-server-types";
import {
plugin as pluginCacheControl,
CacheControlExtensionOptions,
} from 'apollo-cache-control';
import { getEngineApiKey, getEngineGraphVariant } from "apollo-engine-reporting/dist/agent";
import { cloneObject } from "./runHttpQuery";
import isNodeLike from './utils/isNodeLike';

const NoIntrospection = (context: ValidationContext) => ({
Field(node: FieldDefinitionNode) {
Expand Down Expand Up @@ -149,7 +151,7 @@ export class ApolloServerBase {
private config: Config;
/** @deprecated: This is undefined for servers operating as gateways, and will be removed in a future release **/
protected schema?: GraphQLSchema;
private toDispose = new Set<() => void>();
private toDispose = new Set<() => ValueOrPromise<void>>();
private experimental_approximateDocumentStoreMiB:
Config['experimental_approximateDocumentStoreMiB'];

Expand Down Expand Up @@ -177,6 +179,7 @@ export class ApolloServerBase {
gateway,
cacheControl,
experimental_approximateDocumentStoreMiB,
stopOnTerminationSignals,
...requestOptions
} = config;

Expand Down Expand Up @@ -385,6 +388,32 @@ export class ApolloServerBase {
// is populated accordingly.
this.ensurePluginInstantiation(plugins);

// 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. (For backwards
// compatibility, we check both 'stopOnTerminationSignals' and
// 'engine.handleSignals'.)
if (
typeof stopOnTerminationSignals === 'boolean'
? stopOnTerminationSignals
: typeof this.config.engine === 'object' &&
typeof this.config.engine.handleSignals === 'boolean'
? this.config.engine.handleSignals
: isNodeLike && process.env.NODE_ENV !== 'test'
) {
const signals: NodeJS.Signals[] = ['SIGINT', 'SIGTERM'];
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();
process.kill(process.pid, signal);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize this originated in the code from before, but I don't understand why we need send the same signal to ourselves in response to having received that signal.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The theory is that we are not trying to prevent the process from dying when asked to, but just let it do some work before dying as requested.

It's not really a primitive that composes well, but I don't know of a better alternative.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't that still happen if we didn't call process.kill? I'm fine leaving it, it just seemed cyclical (though not in a way that would prevent the server from shutting down.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be confused, but I'm pretty sure that handling a signal means that the signal is handled and the default behavior of the process exiting doesn't occur.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I didn't actually believe that to be the case, but I haven't double clicked on that idea in a while.

};
process.once(signal, handler);
this.toDispose.add(() => {
process.removeListener(signal, handler);
});
});
}
}

// used by integrations to synchronize the path with subscriptions, some
Expand Down Expand Up @@ -585,24 +614,33 @@ export class ApolloServerBase {
if (this.requestOptions.persistedQueries?.cache) {
service.persistedQueries = {
cache: this.requestOptions.persistedQueries.cache,
}
};
}

await Promise.all(
this.plugins.map(
plugin =>
plugin.serverWillStart &&
plugin.serverWillStart(service),
),
const serverListeners = (
await Promise.all(
this.plugins.map(
(plugin) => plugin.serverWillStart && plugin.serverWillStart(service),
),
)
).filter(
(maybeServerListener): maybeServerListener is GraphQLServerListener =>
typeof maybeServerListener === 'object' &&
!!maybeServerListener.serverWillStop,
);
this.toDispose.add(async () => {
await Promise.all(
serverListeners.map(({ serverWillStop }) => serverWillStop?.()),
);
});
Comment on lines +631 to +635
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the additional async/await here is just extraneous (though harmless)

Suggested change
this.toDispose.add(async () => {
await Promise.all(
serverListeners.map(({ serverWillStop }) => serverWillStop?.()),
);
});
this.toDispose.add(() => {
return Promise.all(
serverListeners.map(({ serverWillStop }) => serverWillStop?.()),
);
});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think harmless is probably a true assessment, particularly since this is not a hot-spot in the code from a performance standpoint, but I think it's worth noting that it does create an additional Promise which needs to be resolved and await always yields to the event loop so it would be processed on the next tick. (Might even add an entry to the stack?) If we were doing it often, there might be memory implications. While some runtimes might (eventually) optimize it out via an optimization known as tail call optimization, that is not an optimization that exists in V8 today and it may never land in many runtimes (See link).

Since this is a shutdown mechanism, we might actually be counting down the ticks until we can terminate, though I don't think anything here would be anything more than a microtask so I don't believe we're actually putting anything on the next full turn of the event loop.

I might say that returning the Promise directly is probably preferred. However, to be very clear, this is just me trying to shed light on Promise execution dynamics, not me asking for a change.

Copy link
Member Author

@glasser glasser Aug 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your suggestion here doesn't typecheck because toDispose is supposed to return ValueOrPromise<void> but when I return Promise.all it ends up with ValueOrPromise<(void | undefined)[]>. (The code is semantically different as I had not written return await.) I guess we could change the typing on toDispose, or I could throw in a .then(() => {}) or something, but the async/await version seems clearer to me than either of those, and this is not performance-critical code. Let me know if you disagree!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. .then would work, but not worth changing. Fine with me!

}

public async stop() {
this.toDispose.forEach(dispose => dispose());
await Promise.all([...this.toDispose].map(dispose => dispose()));
if (this.subscriptionServer) await this.subscriptionServer.close();
if (this.engineReportingAgent) {
this.engineReportingAgent.stop();
await this.engineReportingAgent.sendAllReports();
await this.engineReportingAgent.sendAllReportsAndReportErrors();
}
}

Expand Down
1 change: 1 addition & 0 deletions packages/apollo-server-core/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ export interface Config extends BaseConfig {
playground?: PlaygroundConfig;
gateway?: GraphQLService;
experimental_approximateDocumentStoreMiB?: number;
stopOnTerminationSignals?: boolean;
}

export interface FileUploadOptions {
Expand Down
10 changes: 8 additions & 2 deletions packages/apollo-server-core/src/utils/pluginTestHarness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
import {
ApolloServerPlugin,
GraphQLRequestExecutionListener,
GraphQLServerListener,
} from 'apollo-server-plugin-base';
import { InMemoryLRUCache } from 'apollo-server-caching';
import { Dispatcher } from './dispatcher';
Expand Down Expand Up @@ -98,16 +99,19 @@ export default async function pluginTestHarness<TContext>({
}

const schemaHash = generateSchemaHash(schema);
let serverListener: GraphQLServerListener | undefined;
if (typeof pluginInstance.serverWillStart === 'function') {
pluginInstance.serverWillStart({
const maybeServerListener = await pluginInstance.serverWillStart({
logger: logger || console,
schema,
schemaHash,
engine: {},
});
if (maybeServerListener && maybeServerListener.serverWillStop) {
serverListener = maybeServerListener;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, but what are your thoughts of calling maybeListener to pluginInstanceListener? That seems a bit less ambiguous to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to maybeServerListener which matches the type better

}


const requestContext: GraphQLRequestContext<TContext> = {
logger: logger || console,
schema,
Expand Down Expand Up @@ -188,5 +192,7 @@ export default async function pluginTestHarness<TContext>({
requestContext as GraphQLRequestContextWillSendResponse<TContext>,
);

await serverListener?.serverWillStop?.();

return requestContext as GraphQLRequestContextWillSendResponse<TContext>;
}
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ describe('apollo-server-express', () => {
serverOptions: ApolloServerExpressConfig,
options: Partial<ServerRegistration> = {},
) {
server = new ApolloServer(serverOptions);
server = new ApolloServer({stopOnTerminationSignals: false, ...serverOptions});
app = express();

server.applyMiddleware({ ...options, app });
Expand Down Expand Up @@ -184,13 +184,12 @@ describe('apollo-server-express', () => {
});

it('renders GraphQL playground using request original url', async () => {
const nodeEnv = process.env.NODE_ENV;
delete process.env.NODE_ENV;
const samplePath = '/innerSamplePath';

const rewiredServer = new ApolloServer({
typeDefs,
resolvers,
playground: true,
Comment on lines -187 to +192
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this just a hacky workaround to make playground render in a testing env?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking about this as the opposite — setting NODE_ENV to mean "render playground" is a hacky workaround, whereas given that this is a test of playground functionality, asking for what you need makes sense. I left in some tests which set NODE_ENV which are explicitly saying "make sure playground is on by default in production" though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're on the same page. This was a hacky workaround, but I prefer the change you made here. Just confirming my understanding of the change.

});
const innerApp = express();
rewiredServer.applyMiddleware({ app: innerApp });
Expand Down Expand Up @@ -218,7 +217,6 @@ describe('apollo-server-express', () => {
},
},
(error, response, body) => {
process.env.NODE_ENV = nodeEnv;
if (error) {
reject(error);
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ describe('apollo-server-fastify', () => {
options: Partial<ServerRegistration> = {},
mockDecorators: boolean = false,
) {
server = new ApolloServer(serverOptions);
server = new ApolloServer({ stopOnTerminationSignals: false, ...serverOptions });
app = fastify();

if (mockDecorators) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ const port = 0;
server = new ApolloServer({
typeDefs,
resolvers,
stopOnTerminationSignals: false,
});
app = new Server({ port });

Expand Down Expand Up @@ -514,6 +515,7 @@ const port = 0;
server = new ApolloServer({
typeDefs,
resolvers,
stopOnTerminationSignals: false,
context: () => {
throw new AuthenticationError('valid result');
},
Expand Down Expand Up @@ -562,6 +564,7 @@ const port = 0;
},
},
},
stopOnTerminationSignals: false,
});

app = new Server({ port });
Expand Down Expand Up @@ -609,6 +612,7 @@ const port = 0;
},
},
},
stopOnTerminationSignals: false,
});

app = new Server({ port });
Expand Down Expand Up @@ -653,6 +657,7 @@ const port = 0;
},
},
},
stopOnTerminationSignals: false,
});

app = new Server({ port });
Expand Down
Loading