Execute can continue running resolvers even after its returned Promise resolves

[glasser]

(I thought I'd filed this a while ago but apparently not! Came from apollographql/apollo-server#4472 (comment))

graphql-js doesn't proactively short-circuit executions when they are guaranteed to end up ignored by a null-bubbling, as long as the execution actually started due to there being Promises involved.

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 that graphql-js execution will continue running whatever is under ARBITRARY_SELECTION_SET without any sort of short circuiting. (Its response ends up in a Promise that is being captured by a Promise.all which has already rejected.). 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.

In an ideal world, we'd be able to stop processing a selection set as soon as it becomes clear that its results will be ignored. At the very least, it would be nice if we didn't continue running new fields after the overall execute Promise resolves.

[yaacovCR]

WIP at #4263 => @glasser better late than never

[glasser]

@yaacovCR nice! I think I'm a bit too rusty to do a full review without a bunch of research first but let me know if you do need help.

Just as a note — I assume from a skim that this implementation may guarantee "don't start running new fields after the main execution Promise completes" but not "every field that already started has (async) completed" (which is probably a reasonable choice as you don't necessarily want the main response to block on a hanging field when you've aborted due to timeout).

[yaacovCR]

...that this implementation may guarantee "don't start running new fields after the main execution Promise completes" but not "every field that already started has (async) completed" (which is probably a reasonable choice as you don't necessarily want the main response to block on a hanging field when you've aborted due to timeout).

at the time of the writing of your comment, I do not believe those were the guarantees, but with #4267 -- upon which #4263 now depends -- the implementation will match your description. When the abortSignal is triggered, all pending promises should reject immediately, and then #4263 will make sure that any zombie execution does not kick off any new resolvers.

[glasser]

I'm not sure I 100% follow — eg, the abort in Canceller means that anything waiting on the completion of the nested field's value will reject quickly, but that doesn't mean that the resolver itself can't continue running various async actions in series for a while. (But I'm not sure this is a particularly solvable problem, since I think it basically just requires "every time you do an async thing in a resolver it needs to take an AbortSignal explicitly".) Looks like a huge improvement in any case, and probably about as good as imaginable!

[yaacovCR]

Correct. What I haven’t solved here is a resolver that returns synchronously with a list of promises. Those promises can delay the abort return….

[yaacovCR]

Correct. What I haven’t solved here is a resolver that returns synchronously with a list of promises. Those promises can delay the abort return….

That is now also closed by #4267, this issue closed by #4263.