From 06e8d549b427a2d4fcfffcc0a9eb67e78b14509b Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 8 Feb 2021 12:24:32 -0800 Subject: [PATCH 1/3] Make ApolloServer.stop invoke ApolloGateway.stop Because AS is what invokes ApolloGateway.load, it should be its responsibility to invoke the matching stop method. https://github.com/apollographql/federation/pull/452 (aimed at @apollo/gateway 0.23) will change ApolloGateway to no longer "unref" its polling timer: ie, it makes calling `stop()` actually part of its expected API instead of something you can ignore if you feel like it without affecting program shutdown. It has a bit of a hack to still unref the timer if it looks like you're using an old (pre-2.18) version of Apollo Server, but this PR (which will be released in v2.20.0) will make ApolloServer stop the gateway for you. Part of fixing #4428. --- CHANGELOG.md | 1 + .../apollo-server-core/src/ApolloServer.ts | 21 ++++++++++++------- packages/apollo-server-core/src/types.ts | 1 + 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b1b7637661f..a8b115e2add 100644 --- a/CHANGELOG.md +++ b/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`: When used with `ApolloGateway`, `ApolloServer.stop` now invokes `ApolloGateway.stop`. (This makes sense because `ApolloServer` already invokes `ApolloGateway.load` which is what starts the behavior stopped by `ApolloGateway.stop`.) Note that `@apollo/gateway` 0.23 will expect to be stopped in order for natural program shutdown to occur. [Issue #4428](https://github.com/apollographql/apollo-server/issues/4428) - `apollo-server-core`: Avoid instrumenting schemas for the old `graphql-extensions` library unless extensions are provided. [PR #4893](https://github.com/apollographql/apollo-server/pull/4893) [Issue #4889](https://github.com/apollographql/apollo-server/issues/4889) - `apollo-server-plugin-response-cache`: The `shouldReadFromCache` and `shouldWriteToCache` hooks were always documented as returning `ValueOrPromise` (ie, that they could be either sync or async), but they actually only worked if they returned a bool. Now they can be either sync or async as intended. [PR #4890](http://github.com/apollographql/apollo-server/pull/4890) [Issue #4886](https://github.com/apollographql/apollo-server/issues/4886) diff --git a/packages/apollo-server-core/src/ApolloServer.ts b/packages/apollo-server-core/src/ApolloServer.ts index 68b6f9428ed..d7225c02439 100644 --- a/packages/apollo-server-core/src/ApolloServer.ts +++ b/packages/apollo-server-core/src/ApolloServer.ts @@ -392,7 +392,7 @@ export class ApolloServerBase { // Store the unsubscribe handles, which are returned from // `onSchemaChange`, for later disposal when the server stops gateway.onSchemaChange( - schema => + (schema) => (this.schemaDerivedData = Promise.resolve( this.generateSchemaDerivedData(schema), )), @@ -415,17 +415,24 @@ export class ApolloServerBase { // a federated schema! this.requestOptions.executor = gateway.executor; - return gateway.load({ apollo: this.apolloConfig, engine: engineConfig }) - .then(config => config.schema) - .catch(err => { + return gateway + .load({ apollo: this.apolloConfig, engine: engineConfig }) + .then((config) => { + this.toDispose.add( + async () => gateway.stop && (await gateway.stop()), + ); + return config.schema; + }) + .catch((err) => { // We intentionally do not re-throw the exact error from the gateway // configuration as it may contain implementation details and this // error will propagate to the client. We will, however, log the error // for observation in the logs. - const message = "This data graph is missing a valid configuration."; - this.logger.error(message + " " + (err && err.message || err)); + const message = 'This data graph is missing a valid configuration.'; + this.logger.error(message + ' ' + ((err && err.message) || err)); throw new Error( - message + " More details may be available in the server logs."); + message + ' More details may be available in the server logs.', + ); }); } diff --git a/packages/apollo-server-core/src/types.ts b/packages/apollo-server-core/src/types.ts index b7b59b63c00..ec0385c99da 100644 --- a/packages/apollo-server-core/src/types.ts +++ b/packages/apollo-server-core/src/types.ts @@ -99,6 +99,7 @@ export interface GraphQLService { executor( requestContext: GraphQLRequestContextExecutionDidStart, ): ValueOrPromise; + stop?(): Promise; } // This configuration is shared between all integrations and should include From e1090b2e903b63edcad3b0fa8abb251dbfa60365 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 8 Feb 2021 13:48:58 -0800 Subject: [PATCH 2/3] Simplify typings of toDispose set `() => ValueOrPromise` is a weird type because `void` means "I'm not going to look at the return value" which is sorta incompatible with "but I need to see if it's a Promise or not". So just make these all async (changing a couple implementations by adding `async`). --- .../apollo-server-core/src/ApolloServer.ts | 27 +++++++++---------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/packages/apollo-server-core/src/ApolloServer.ts b/packages/apollo-server-core/src/ApolloServer.ts index d7225c02439..fbd15527219 100644 --- a/packages/apollo-server-core/src/ApolloServer.ts +++ b/packages/apollo-server-core/src/ApolloServer.ts @@ -148,7 +148,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<() => ValueOrPromise>(); + private toDispose = new Set<() => Promise>(); private experimental_approximateDocumentStoreMiB: Config['experimental_approximateDocumentStoreMiB']; @@ -364,7 +364,7 @@ export class ApolloServerBase { process.kill(process.pid, signal); }; process.once(signal, handler); - this.toDispose.add(() => { + this.toDispose.add(async () => { process.removeListener(signal, handler); }); }); @@ -388,16 +388,15 @@ export class ApolloServerBase { parseOptions, } = this.config; if (gateway) { - this.toDispose.add( - // Store the unsubscribe handles, which are returned from - // `onSchemaChange`, for later disposal when the server stops - gateway.onSchemaChange( - (schema) => - (this.schemaDerivedData = Promise.resolve( - this.generateSchemaDerivedData(schema), - )), - ), + // Store the unsubscribe handles, which are returned from + // `onSchemaChange`, for later disposal when the server stops + const unsubscriber = gateway.onSchemaChange( + (schema) => + (this.schemaDerivedData = Promise.resolve( + this.generateSchemaDerivedData(schema), + )), ); + this.toDispose.add(async () => unsubscriber()); // For backwards compatibility with old versions of @apollo/gateway. const engineConfig = @@ -418,9 +417,7 @@ export class ApolloServerBase { return gateway .load({ apollo: this.apolloConfig, engine: engineConfig }) .then((config) => { - this.toDispose.add( - async () => gateway.stop && (await gateway.stop()), - ); + this.toDispose.add(async () => await gateway.stop?.()); return config.schema; }) .catch((err) => { @@ -600,7 +597,7 @@ export class ApolloServerBase { public async stop() { await Promise.all([...this.toDispose].map(dispose => dispose())); - if (this.subscriptionServer) await this.subscriptionServer.close(); + if (this.subscriptionServer) this.subscriptionServer.close(); } public installSubscriptionHandlers(server: HttpServer | HttpsServer | Http2Server | Http2SecureServer | WebSocket.Server) { From 2587a91f7145ea39d342c368f34867f2fac11b9f Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 8 Feb 2021 13:56:24 -0800 Subject: [PATCH 3/3] compile --- packages/apollo-server-core/src/ApolloServer.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/apollo-server-core/src/ApolloServer.ts b/packages/apollo-server-core/src/ApolloServer.ts index fbd15527219..c599a3b9e47 100644 --- a/packages/apollo-server-core/src/ApolloServer.ts +++ b/packages/apollo-server-core/src/ApolloServer.ts @@ -76,7 +76,7 @@ import { import { Headers } from 'apollo-server-env'; import { buildServiceDefinition } from '@apollographql/apollo-tools'; import { plugin as pluginTracing } from "apollo-tracing"; -import { Logger, SchemaHash, ValueOrPromise, ApolloConfig } from "apollo-server-types"; +import { Logger, SchemaHash, ApolloConfig } from "apollo-server-types"; import { plugin as pluginCacheControl, CacheControlExtensionOptions,