From e927698550d84a241d665baa8887397d617d0d6b Mon Sep 17 00:00:00 2001 From: David Glasser Date: Tue, 4 Aug 2020 01:02:18 -0700 Subject: [PATCH] Add serverWillStop lifecycle hook; call stop() on signals by default Fixes #4273. This PR adds a serverWillStop plugin lifecycle hook. The `serverWillStop` hook is on an object optionally returned from a `serverWillStart` hook, similar to `executionDidStart`/`executionDidEnd`. ApolloServerPluginOperationRegistry uses this to stop its agent. The code that installs SIGINT and SIGTERM handlers unless disabled with `handleSignals: false` is hoisted from EngineReportingAgent to ApolloServer itself and renamed to `stopOnTerminationSignals` as a new ApolloServer option. The new implementation also skips installing the signals handlers by default if NODE_ENV=test or if you don't appear to be running in Node (and we update some tests that explicitly set other NODE_ENVs to set handleSignals: false). The main effect on existing code is that on one of these signals, any SubscriptionServer and ApolloGateway will be stopped in addition to any EngineReportingAgent. --- docs/source/api/apollo-server.md | 18 ++++-- packages/apollo-engine-reporting/src/agent.ts | 31 +--------- .../apollo-server-core/src/ApolloServer.ts | 60 +++++++++++++++---- packages/apollo-server-core/src/types.ts | 1 + .../src/utils/pluginTestHarness.ts | 10 +++- .../src/__tests__/ApolloServer.test.ts | 6 +- .../src/__tests__/ApolloServer.test.ts | 2 +- .../src/__tests__/ApolloServer.test.ts | 5 ++ .../src/ApolloServer.ts | 8 +++ .../src/__tests__/ApolloServer.test.ts | 2 +- .../src/__tests__/ApolloServer.test.ts | 6 +- .../apollo-server-plugin-base/src/index.ts | 10 +++- .../ApolloServerPluginOperationRegistry.ts | 9 ++- .../apollo-server/src/__tests__/index.test.ts | 30 ++++++++++ 14 files changed, 142 insertions(+), 56 deletions(-) diff --git a/docs/source/api/apollo-server.md b/docs/source/api/apollo-server.md index 0f06576b04c..4cf79dc2be5 100644 --- a/docs/source/api/apollo-server.md +++ b/docs/source/api/apollo-server.md @@ -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` @@ -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 + equivalent to specifying `new ApolloServer({stopOnTerminationSignals: false})`. * `rewriteError`: (err: GraphQLError) => GraphQLError | null diff --git a/packages/apollo-engine-reporting/src/agent.ts b/packages/apollo-engine-reporting/src/agent.ts index cca8cb0c4bf..5105bfbcd93 100644 --- a/packages/apollo-engine-reporting/src/agent.ts +++ b/packages/apollo-engine-reporting/src/agent.ts @@ -290,11 +290,8 @@ export interface EngineReportingOptions { */ privateHeaders?: Array | 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; /** @@ -445,8 +442,6 @@ export class EngineReportingAgent { private stopped: boolean = false; private signatureCache: InMemoryLRUCache; - private signalHandlers = new Map(); - private currentSchemaReporter?: SchemaReporter; private readonly bootId: string; private lastSeenExecutableSchemaToId?: { @@ -529,21 +524,6 @@ export class EngineReportingAgent { ); } - 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`.', @@ -847,11 +827,6 @@ export class EngineReportingAgent { // 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; @@ -930,7 +905,7 @@ export class EngineReportingAgent { return generatedSignature; } - private async sendAllReportsAndReportErrors(): Promise { + public async sendAllReportsAndReportErrors(): Promise { await Promise.all( Object.keys(this.reportDataByExecutableSchemaId).map(executableSchemaId => this.sendReportAndReportErrors(executableSchemaId), diff --git a/packages/apollo-server-core/src/ApolloServer.ts b/packages/apollo-server-core/src/ApolloServer.ts index c903c9722ee..eda7a85adce 100644 --- a/packages/apollo-server-core/src/ApolloServer.ts +++ b/packages/apollo-server-core/src/ApolloServer.ts @@ -31,6 +31,7 @@ import { import { ApolloServerPlugin, GraphQLServiceContext, + GraphQLServerListener, } from 'apollo-server-plugin-base'; import runtimeSupportsUploads from './utils/runtimeSupportsUploads'; @@ -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) { @@ -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>(); private experimental_approximateDocumentStoreMiB: Config['experimental_approximateDocumentStoreMiB']; @@ -177,6 +179,7 @@ export class ApolloServerBase { gateway, cacheControl, experimental_approximateDocumentStoreMiB, + stopOnTerminationSignals, ...requestOptions } = config; @@ -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); + }; + process.once(signal, handler); + this.toDispose.add(() => { + process.removeListener(signal, handler); + }); + }); + } } // used by integrations to synchronize the path with subscriptions, some @@ -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?.()), + ); + }); } 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(); } } diff --git a/packages/apollo-server-core/src/types.ts b/packages/apollo-server-core/src/types.ts index 86f24ba434f..491be13340a 100644 --- a/packages/apollo-server-core/src/types.ts +++ b/packages/apollo-server-core/src/types.ts @@ -124,6 +124,7 @@ export interface Config extends BaseConfig { playground?: PlaygroundConfig; gateway?: GraphQLService; experimental_approximateDocumentStoreMiB?: number; + stopOnTerminationSignals?: boolean; } export interface FileUploadOptions { diff --git a/packages/apollo-server-core/src/utils/pluginTestHarness.ts b/packages/apollo-server-core/src/utils/pluginTestHarness.ts index 9f2d9f51e3d..af034f07e5e 100644 --- a/packages/apollo-server-core/src/utils/pluginTestHarness.ts +++ b/packages/apollo-server-core/src/utils/pluginTestHarness.ts @@ -17,6 +17,7 @@ import { import { ApolloServerPlugin, GraphQLRequestExecutionListener, + GraphQLServerListener, } from 'apollo-server-plugin-base'; import { InMemoryLRUCache } from 'apollo-server-caching'; import { Dispatcher } from './dispatcher'; @@ -98,16 +99,19 @@ export default async function pluginTestHarness({ } 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; + } } - const requestContext: GraphQLRequestContext = { logger: logger || console, schema, @@ -188,5 +192,7 @@ export default async function pluginTestHarness({ requestContext as GraphQLRequestContextWillSendResponse, ); + await serverListener?.serverWillStop?.(); + return requestContext as GraphQLRequestContextWillSendResponse; } diff --git a/packages/apollo-server-express/src/__tests__/ApolloServer.test.ts b/packages/apollo-server-express/src/__tests__/ApolloServer.test.ts index 69e5598ef09..b78254c1bdd 100644 --- a/packages/apollo-server-express/src/__tests__/ApolloServer.test.ts +++ b/packages/apollo-server-express/src/__tests__/ApolloServer.test.ts @@ -62,7 +62,7 @@ describe('apollo-server-express', () => { serverOptions: ApolloServerExpressConfig, options: Partial = {}, ) { - server = new ApolloServer(serverOptions); + server = new ApolloServer({stopOnTerminationSignals: false, ...serverOptions}); app = express(); server.applyMiddleware({ ...options, app }); @@ -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, }); const innerApp = express(); rewiredServer.applyMiddleware({ app: innerApp }); @@ -218,7 +217,6 @@ describe('apollo-server-express', () => { }, }, (error, response, body) => { - process.env.NODE_ENV = nodeEnv; if (error) { reject(error); } else { diff --git a/packages/apollo-server-fastify/src/__tests__/ApolloServer.test.ts b/packages/apollo-server-fastify/src/__tests__/ApolloServer.test.ts index b31ea534ace..c38730cd47f 100644 --- a/packages/apollo-server-fastify/src/__tests__/ApolloServer.test.ts +++ b/packages/apollo-server-fastify/src/__tests__/ApolloServer.test.ts @@ -64,7 +64,7 @@ describe('apollo-server-fastify', () => { options: Partial = {}, mockDecorators: boolean = false, ) { - server = new ApolloServer(serverOptions); + server = new ApolloServer({ stopOnTerminationSignals: false, ...serverOptions }); app = fastify(); if (mockDecorators) { diff --git a/packages/apollo-server-hapi/src/__tests__/ApolloServer.test.ts b/packages/apollo-server-hapi/src/__tests__/ApolloServer.test.ts index 2ced8e8c66e..7699878bcbc 100644 --- a/packages/apollo-server-hapi/src/__tests__/ApolloServer.test.ts +++ b/packages/apollo-server-hapi/src/__tests__/ApolloServer.test.ts @@ -155,6 +155,7 @@ const port = 0; server = new ApolloServer({ typeDefs, resolvers, + stopOnTerminationSignals: false, }); app = new Server({ port }); @@ -514,6 +515,7 @@ const port = 0; server = new ApolloServer({ typeDefs, resolvers, + stopOnTerminationSignals: false, context: () => { throw new AuthenticationError('valid result'); }, @@ -562,6 +564,7 @@ const port = 0; }, }, }, + stopOnTerminationSignals: false, }); app = new Server({ port }); @@ -609,6 +612,7 @@ const port = 0; }, }, }, + stopOnTerminationSignals: false, }); app = new Server({ port }); @@ -653,6 +657,7 @@ const port = 0; }, }, }, + stopOnTerminationSignals: false, }); app = new Server({ port }); diff --git a/packages/apollo-server-integration-testsuite/src/ApolloServer.ts b/packages/apollo-server-integration-testsuite/src/ApolloServer.ts index bcc0caf3996..1e709261188 100644 --- a/packages/apollo-server-integration-testsuite/src/ApolloServer.ts +++ b/packages/apollo-server-integration-testsuite/src/ApolloServer.ts @@ -233,6 +233,7 @@ export function testApolloServer( const { url: uri } = await createApolloServer({ schema, + stopOnTerminationSignals: false, }); const apolloFetch = createApolloFetch({ uri }); @@ -250,6 +251,7 @@ export function testApolloServer( const { url: uri } = await createApolloServer({ schema, + stopOnTerminationSignals: false, }); const apolloFetch = createApolloFetch({ uri }); @@ -272,6 +274,7 @@ export function testApolloServer( const { url: uri } = await createApolloServer({ schema, introspection: true, + stopOnTerminationSignals: false, }); const apolloFetch = createApolloFetch({ uri }); @@ -1009,6 +1012,7 @@ export function testApolloServer( }, formatError, debug: true, + stopOnTerminationSignals: false, }); const apolloFetch = createApolloFetch({ uri }); @@ -1078,6 +1082,7 @@ export function testApolloServer( ...engineOptions, }, debug: true, + stopOnTerminationSignals: false, ...constructorOptions, }); @@ -1682,6 +1687,7 @@ export function testApolloServer( const { url: uri } = await createApolloServer({ typeDefs, resolvers, + stopOnTerminationSignals: false, context: () => { throw new AuthenticationError('valid result'); }, @@ -1743,6 +1749,7 @@ export function testApolloServer( }, }, }, + stopOnTerminationSignals: false, }); const apolloFetch = createApolloFetch({ uri }); @@ -1776,6 +1783,7 @@ export function testApolloServer( }, }, }, + stopOnTerminationSignals: false, }); const apolloFetch = createApolloFetch({ uri }); diff --git a/packages/apollo-server-koa/src/__tests__/ApolloServer.test.ts b/packages/apollo-server-koa/src/__tests__/ApolloServer.test.ts index 35d99376164..af8374b9e59 100644 --- a/packages/apollo-server-koa/src/__tests__/ApolloServer.test.ts +++ b/packages/apollo-server-koa/src/__tests__/ApolloServer.test.ts @@ -77,7 +77,7 @@ const resolvers = { serverOptions: Config, options: Partial = {}, ) { - server = new ApolloServer(serverOptions); + server = new ApolloServer({ stopOnTerminationSignals: false, ...serverOptions }); app = new Koa(); server.applyMiddleware({ ...options, app }); diff --git a/packages/apollo-server-micro/src/__tests__/ApolloServer.test.ts b/packages/apollo-server-micro/src/__tests__/ApolloServer.test.ts index 941242f1283..037b9586b88 100644 --- a/packages/apollo-server-micro/src/__tests__/ApolloServer.test.ts +++ b/packages/apollo-server-micro/src/__tests__/ApolloServer.test.ts @@ -22,7 +22,11 @@ const resolvers = { }; async function createServer(options: object = {}): Promise { - const apolloServer = new ApolloServer({ typeDefs, resolvers }); + const apolloServer = new ApolloServer({ + typeDefs, + resolvers, + stopOnTerminationSignals: false, + }); const service = micro(apolloServer.createHandler(options)); const uri = await listen(service); return { diff --git a/packages/apollo-server-plugin-base/src/index.ts b/packages/apollo-server-plugin-base/src/index.ts index a77d6556c16..e6a71b36dcd 100644 --- a/packages/apollo-server-plugin-base/src/index.ts +++ b/packages/apollo-server-plugin-base/src/index.ts @@ -55,13 +55,19 @@ export { export interface ApolloServerPlugin< TContext extends BaseContext = BaseContext -> { - serverWillStart?(service: GraphQLServiceContext): ValueOrPromise; +> extends AnyFunctionMap { + serverWillStart?( + service: GraphQLServiceContext, + ): ValueOrPromise; requestDidStart?( requestContext: GraphQLRequestContext, ): GraphQLRequestListener | void; } +export interface GraphQLServerListener { + serverWillStop?(): ValueOrPromise; +} + export type GraphQLRequestListenerParsingDidEnd = (err?: Error) => void; export type GraphQLRequestListenerValidationDidEnd = ((err?: ReadonlyArray) => void); diff --git a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts index 7d4c25305f9..0200d75e3bf 100644 --- a/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts +++ b/packages/apollo-server-plugin-operation-registry/src/ApolloServerPluginOperationRegistry.ts @@ -4,6 +4,7 @@ import { GraphQLServiceContext, GraphQLRequestListener, GraphQLRequestContext, + GraphQLServerListener, } from 'apollo-server-plugin-base'; import { /** @@ -97,7 +98,7 @@ for observability purposes, but all operations will be permitted.`, return (): ApolloServerPlugin => ({ async serverWillStart({ engine, - }: GraphQLServiceContext): Promise { + }: GraphQLServiceContext): Promise { logger.debug('Initializing operation registry plugin.'); if (!engine || !engine.serviceID) { @@ -124,6 +125,12 @@ for observability purposes, but all operations will be permitted.`, }); await agent.start(); + + return { + serverWillStop() { + agent.stop(); + }, + }; }, requestDidStart(): GraphQLRequestListener { diff --git a/packages/apollo-server/src/__tests__/index.test.ts b/packages/apollo-server/src/__tests__/index.test.ts index b5d4982fec5..c670a6f3fbb 100644 --- a/packages/apollo-server/src/__tests__/index.test.ts +++ b/packages/apollo-server/src/__tests__/index.test.ts @@ -25,6 +25,35 @@ describe('apollo-server', () => { expect(() => new ApolloServer({ typeDefs, mocks: true })).not.toThrow; }); + it('runs serverWillStart and serverWillStop', async () => { + const fn = jest.fn(); + const beAsync = () => new Promise((res) => res()); + const server = new ApolloServer({ + typeDefs, + resolvers, + plugins: [ + { + async serverWillStart() { + fn('a'); + await beAsync(); + fn('b'); + return { + async serverWillStop() { + fn('c'); + await beAsync(); + fn('d'); + }, + }; + }, + }, + ], + }); + await server.listen(); + expect(fn.mock.calls).toEqual([['a'], ['b']]); + await server.stop(); + expect(fn.mock.calls).toEqual([['a'], ['b'], ['c'], ['d']]); + }); + // These tests are duplicates of ones in apollo-server-integration-testsuite // We don't actually expect Jest to do much here, the purpose of these // tests is to make sure our typings are correct, and to trigger a @@ -102,6 +131,7 @@ describe('apollo-server', () => { server = new ApolloServer({ typeDefs, resolvers, + stopOnTerminationSignals: false, }); const { url } = await server.listen({ port: 0 });