Error handling in plugins is not consistent and can lead to plugin methods not being called
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 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) - 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 dummyasync (..args) { await f(...args) }or the equivalent. - Ideally
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. - We should never pass errors thrown by
executionDidEndback toexecutionDidEnd - If a
DidStarthook (whetherexecutionDidStartorinvokeDidStartHook-based) throws, we should call thedidEndsfrom any previous successful hooks rather than "leaking" it. - We should consider whether we want to always call all
DidStarthooks (includingexecutionDidStart) even if an earlier one throws; plugin installation ordering probably shouldn't have such a major impact here.
This is to a great degree simplified in Apollo Server 3, because nearly all hooks are async (so there's no concern about "what if a correctly implemented hook throws synchronously"). We've also fixed the executionDidEnd multiple-call issue in AS4. But some of these bullets still apply.