apollo-server icon indicating copy to clipboard operation
apollo-server copied to clipboard

Error handling in plugins is not consistent and can lead to plugin methods not being called

Open glasser opened this issue 5 years ago • 1 comments

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.

glasser avatar Dec 30 '20 20:12 glasser

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.

glasser avatar Sep 23 '22 23:09 glasser