From ddce036e1b683adc636a7132e0c249690bf05ce0 Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Wed, 4 Oct 2023 10:47:27 -0700 Subject: [PATCH] (AS4 + #6398): Usage reporting: don't throw errors if `willResolveField` is called "late" (#7747) Porting #6398 from AS3. This fix was unintentionally left out of AS4. Fixes #7746 The minimum version of `graphql` officially supported by Apollo Server 4 as a peer dependency, v16.6.0, contains a [serious bug that can crash your Node server](https://github.com/graphql/graphql-js/issues/3528). This bug is fixed in the immediate next version, `graphql@16.7.0`, and we **strongly encourage you to upgrade your installation of `graphql` to at least v16.7.0** to avoid this bug. (For backwards compatibility reasons, we cannot change Apollo Server 4's minimum peer dependency, but will change it when we release Apollo Server 5.) Apollo Server 4 contained a particular line of code that makes triggering this crashing bug much more likely. This line was already removed in Apollo Server v3.8.2 (see #6398) but the fix was accidentally not included in Apollo Server 4. We are now including this change in Apollo Server 4, which will **reduce** the likelihood of hitting this crashing bug for users of `graphql` v16.6.0. That said, taking this `@apollo/server` upgrade **does not prevent** this bug from being triggered in other ways, and the real fix to this crashing bug is to upgrade `graphql`. --------- Co-authored-by: David Glasser --- .changeset/late-coats-fry.md | 7 +++ docs/source/migration.mdx | 2 +- .../server/src/plugin/traceTreeBuilder.ts | 44 ++++++++++++++++++- 3 files changed, 51 insertions(+), 2 deletions(-) create mode 100644 .changeset/late-coats-fry.md diff --git a/.changeset/late-coats-fry.md b/.changeset/late-coats-fry.md new file mode 100644 index 00000000000..e55194591ec --- /dev/null +++ b/.changeset/late-coats-fry.md @@ -0,0 +1,7 @@ +--- +'@apollo/server': patch +--- + +The minimum version of `graphql` officially supported by Apollo Server 4 as a peer dependency, v16.6.0, contains a [serious bug that can crash your Node server](https://github.com/graphql/graphql-js/issues/3528). This bug is fixed in the immediate next version, `graphql@16.7.0`, and we **strongly encourage you to upgrade your installation of `graphql` to at least v16.7.0** to avoid this bug. (For backwards compatibility reasons, we cannot change Apollo Server 4's minimum peer dependency, but will change it when we release Apollo Server 5.) + +Apollo Server 4 contained a particular line of code that makes triggering this crashing bug much more likely. This line was already removed in Apollo Server v3.8.2 (see #6398) but the fix was accidentally not included in Apollo Server 4. We are now including this change in Apollo Server 4, which will **reduce** the likelihood of hitting this crashing bug for users of `graphql` v16.6.0. That said, taking this `@apollo/server` upgrade **does not prevent** this bug from being triggered in other ways, and the real fix to this crashing bug is to upgrade `graphql`. diff --git a/docs/source/migration.mdx b/docs/source/migration.mdx index 7a5539d2f25..05a8404b484 100644 --- a/docs/source/migration.mdx +++ b/docs/source/migration.mdx @@ -379,7 +379,7 @@ If you're using Node.js 12, upgrade your runtime before upgrading to Apollo Serv ### `graphql` -Apollo Server has a peer dependency on [`graphql`](https://www.npmjs.com/package/graphql) (the core JS GraphQL implementation). Apollo Server 4 supports `graphql` v16.6.0 and later. (Apollo Server 3 supports `graphql` v15.3.0 through v16.) +Apollo Server has a peer dependency on [`graphql`](https://www.npmjs.com/package/graphql) (the core JS GraphQL implementation). Apollo Server 4 supports `graphql` v16.6.0 and later, but we *strongly* recommend using at least v16.7.0 due to a [bug in `graphql`](https://github.com/graphql/graphql-js/issues/3528) which can crash your server. (Apollo Server 3 supports `graphql` v15.3.0 through v16.) If you're using an older version of `graphql`, upgrade it to a supported version before upgrading to Apollo Server 4. diff --git a/packages/server/src/plugin/traceTreeBuilder.ts b/packages/server/src/plugin/traceTreeBuilder.ts index a2aa6eaa867..7d323d46277 100644 --- a/packages/server/src/plugin/traceTreeBuilder.ts +++ b/packages/server/src/plugin/traceTreeBuilder.ts @@ -85,7 +85,49 @@ export class TraceTreeBuilder { throw internalError('willResolveField called before startTiming!'); } if (this.stopped) { - throw internalError('willResolveField called after stopTiming!'); + // We've been stopped, which means execution is done... and yet we're + // still resolving more fields? How can that be? Shouldn't we throw an + // error or something? + // + // Well... we used to do exactly that. But this "shouldn't happen" error + // showed up in practice! Turns out that graphql-js can actually continue + // to execute more fields indefinitely long after `execute()` resolves! + // That's because parallelism on a selection set is implemented using + // `Promise.all`, and as soon as one of its arguments (ie, one field) + // throws an error, the combined Promise resolves, but there's no + // "cancellation" of the Promises that are the other arguments to + // `Promise.all`. So the code contributing to those Promises keeps on + // chugging away indefinitely. + // + // Concrete example: let’s say you have + // + // { x y { ARBITRARY_SELECTION_SET } } + // + // where x has a non-null return type, and x and y both have resolvers + // that return Promises. And let’s say that x returns a Promise that + // rejects (or resolves to null). What this means is that we’re going to + // eventually end up with `data: null` (nothing under y will actually + // matter), but graphql-js execution will continue running whatever is + // under ARBITRARY_SELECTION_SET without any sort of short circuiting. In + // fact, the Promise returned from execute itself can happily resolve + // while execution is still chugging away on an arbitrary amount of fields + // under that ARBITRARY_SELECTION_SET. There’s no way to detect from the + // outside "all the execution related to this operation is done", nor to + // "short-circuit" execution so that it stops going. + // + // So, um. We will record any field whose execution we manage to observe + // before we "stop" the TraceTreeBuilder (whether it is one that actually + // ends up in the response or whether it gets thrown away due to null + // bubbling), but if we get any more fields afterwards, we just ignore + // them rather than throwing a confusing error. + // + // (That said, the error we used to throw here generally was hidden + // anyway, for the same reason: it comes from a branch of execution that + // ends up not being included in the response. But + // https://github.com/graphql/graphql-js/pull/3529 means that this + // sometimes crashed execution anyway. Our error never caught any actual + // problematic cases, so keeping it around doesn't really help.) + return () => {}; } const path = info.path;