Plugin method invocation goes through the Dispatcher class. What happens if a plugin method throws is not consistent and can lead to confusing behavior.
We discovered this specifically when a didEncounterErrors hook threw synchronously which led to ApolloServerPluginUsageReporting's didEncounterErrors hook not being called at all. Effectively, no errors were included in usage reports. This depended on the order of listing plugins (or, if the plugin was being installed at the front via the legacy engine handling vs at the back manually).
Here's how the different hooks work:
invokeHookAsync (didResolveSource, didResolveOperation, willSendResponse, didEncounterErrors): These callbacks return "value or Promise" which honestly is a strange concept and we should have just made them always async, in retrospect. If all hooks actually are async (only return Promise, never directly throw) then all hooks will be called; if more than one promise rejects, all but one rejection will be silently eaten by Promise.all. However, if any hook throws synchronously, plugins farther down on the plugin list will never even have their hook called at all.
invokeHookSync (executionDidStart) and reverseInvokeHookSync (executionDidEnd): If any hook here throws, hooks later on the list (or earlier for executionDidEnd) will not run at all. There's also something weird with the particular call to executionDidEnd: if we're calling it because execute ran without throwing (perhaps returning GraphQL errors) and executionDidEnd itself throws, we end up calling executionDidEnd again (perhaps re-calling the same hooks!) with that error.
invokeHooksUntilNonNull (responseForOperation): if one throws, the rest aren't run; this seems reasonable enough given that "if one returns non-null, the rest aren't run" is expected behavior.
invokeDidStartHook (parsingDidStart, validationDidStart, willResolveField): These run synchronously and if one throws the rest don't run. (And there's no "unwinding" — if one throws then the "didEnd" calls from previously run hooks are never run)
- The did-ends from
invokeDidStartHook: if one throws, then the rest (from earlier plugins) don't get called
There are a lot of potential improvements here:
invokeHookAsync's error handling behavior really should be consistent between sync and async implementations. ie, we should probably wrap every hook in a dummy async (..args) { await f(...args) } or the equivalent.
- Ideally
invokeHookAsync shouldn't make rejections vanish if they aren't the only rejection — it should use something like (a polyfill for) Promise.allSettled followed by throwing a combined error if there are multiple failures.
- We should never pass errors thrown by
executionDidEnd back to executionDidEnd
- If a
DidStart hook (whether executionDidStart or invokeDidStartHook-based) throws, we should call the didEnds from any previous successful hooks rather than "leaking" it.
- We should consider whether we want to always call all
DidStart hooks (including executionDidStart) even if an earlier one throws; plugin installation ordering probably shouldn't have such a major impact here.
Plugin method invocation goes through the Dispatcher class. What happens if a plugin method throws is not consistent and can lead to confusing behavior.
We discovered this specifically when a didEncounterErrors hook threw synchronously which led to ApolloServerPluginUsageReporting's didEncounterErrors hook not being called at all. Effectively, no errors were included in usage reports. This depended on the order of listing plugins (or, if the plugin was being installed at the front via the legacy
enginehandling vs at the back manually).Here's how the different hooks work:
invokeHookAsync(didResolveSource,didResolveOperation,willSendResponse,didEncounterErrors): These callbacks return "value or Promise" which honestly is a strange concept and we should have just made them always async, in retrospect. If all hooks actually are async (only return Promise, never directly throw) then all hooks will be called; if more than one promise rejects, all but one rejection will be silently eaten byPromise.all. However, if any hook throws synchronously, plugins farther down on the plugin list will never even have their hook called at all.invokeHookSync(executionDidStart) andreverseInvokeHookSync(executionDidEnd): If any hook here throws, hooks later on the list (or earlier forexecutionDidEnd) will not run at all. There's also something weird with the particular call toexecutionDidEnd: if we're calling it becauseexecuteran without throwing (perhaps returning GraphQL errors) andexecutionDidEnditself throws, we end up callingexecutionDidEndagain (perhaps re-calling the same hooks!) with that error.invokeHooksUntilNonNull(responseForOperation): if one throws, the rest aren't run; this seems reasonable enough given that "if one returns non-null, the rest aren't run" is expected behavior.invokeDidStartHook(parsingDidStart,validationDidStart,willResolveField): These run synchronously and if one throws the rest don't run. (And there's no "unwinding" — if one throws then the "didEnd" calls from previously run hooks are never run)invokeDidStartHook: if one throws, then the rest (from earlier plugins) don't get calledThere are a lot of potential improvements here:
invokeHookAsync's error handling behavior really should be consistent between sync and async implementations. ie, we should probably wrap every hook in a dummyasync (..args) { await f(...args) }or the equivalent.invokeHookAsyncshouldn't make rejections vanish if they aren't the only rejection — it should use something like (a polyfill for)Promise.allSettledfollowed by throwing a combined error if there are multiple failures.executionDidEndback toexecutionDidEndDidStarthook (whetherexecutionDidStartorinvokeDidStartHook-based) throws, we should call thedidEndsfrom any previous successful hooks rather than "leaking" it.DidStarthooks (includingexecutionDidStart) even if an earlier one throws; plugin installation ordering probably shouldn't have such a major impact here.