apollographql / apollographql/apollo-server
Error handling in plugins is not consistent and can lead to plugin methods not being called
- Dominant language
- TypeScript
- Stars
- 14k
- Forks
- 2k
- Avg merge
- 2d 14h
- Merged PRs (30d)
- 2
Description
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.
Contributor guide
Research direction
Start at the Dispatcher class and trace invokeHookAsync, invokeHookSync, reverseInvokeHookSync, invokeHooksUntilNonNull, and invokeDidStartHook. Compare synchronous throws, rejected promises, and did-end behavior across the listed plugin hooks. Done should mean the chosen error-handling behavior is consistent and prevents later hooks or cleanup hooks from being skipped or called again.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- graphql, typescript
- Domain
- api, backend
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100