Errors thrown when iterating over subscription source event streams (AsyncIterables) should be caught
- Dominant language
- TypeScript
- Stars
- 20.3k
- Forks
- 2.1k
- Avg merge
- 44m
- Merged PRs (30d)
- 6
Description
### Context
Hi there. We're using `graphql-js` and serving subscriptions over WebSocket via [`graphql-ws`](https://github.com/enisdenjo/graphql-ws) (as recommended by Apollo for both server and client).
In our subscriptions' `subscribe` methods, we always return an `AsyncIterable` pretty much right away. We typically do this either by defining our methods via async generator functions (`async function*`), or by calling [`graphql-redis-subscriptions`](https://github.com/davidyaha/graphql-redis-subscriptions)'s `asyncIterator` method. Our `subscribe` methods effectively never throw an error just providing an `AsyncIterable`.
However, we occasionally hit errors actually streaming subscription _events_, when `graphql-js` calls our `AsyncIterable`'s `next()` method. E.g. Redis could be momentarily down, or an upstream producer/generator could fail/throw. So we sometimes `throw` errors during iteration. And importantly, this can happen _mid_-stream.
### Problem
**`graphql-js` does not try/catch/handle errors when iterating over an `AsyncIterable`:**
https://github.com/graphql/graphql-js/blob/2aedf25e157d1d1c8fdfeaa4c0d2f3d9d3457dba/src/execution/mapAsyncIterable.ts#L38-L40
There's even a test case today that explicitly expects _these_ errors to be re-thrown:
https://github.com/graphql/graphql-js/blob/8a95335f545024c09abfa0f07cc326f73a0e466f/src/execution/__tests__/subscribe-test.ts#L1043-L1047
`graphql-ws` doesn't try/catch/handle errors thrown during iteration either:
https://github.com/enisdenjo/graphql-ws/blob/e4a75cc59012cad019fa3711287073a4aef9ed05/src/server.ts#L813-L815
As a result, when occasional errors happen like this, **the entire underlying WebSocket connection is closed.**
This is obviously not good! 😅 This interrupts every other subscription the client may be subscribed to at that moment, adds reconnection overhead, drops events, etc. And if we're experiencing some downtime on a specific subscription/source stream, this'll result in repeat disconnect-reconnect thrash, because the client also has no signal on _which_ subscription has failed!!
### Inconsistency
You could argue that `graphql-ws` should try/catch these errors and send back an `error` message itself. The author of `graphql-ws` believes this is the domain of `graphql-js`, though (https://github.com/enisdenjo/graphql-ws/discussions/333), and I agree.
That's because `graphql-js` already try/catches and handles errors both _earlier_ in the execution of a subscription _and later_:
* Errors _producing an `AsyncIterable` in the first place_ (the synchronous result of calling the subscription's `subscribe` method, AKA producing a source event stream in the spec) are caught, and returned as a `{data: null, errors: ...}` result:
https://github.com/graphql/graphql-js/blob/2aedf25e157d1d1c8fdfeaa4c0d2f3d9d3457dba/src/execution/execute.ts#L1784-L1793
* Errors _mapping iteration results to response events_ (the result of calling the subscription's `resolve` method) are caught, and sent back to the client as a `{value: {data: null, errors: ...}, done: false}` event:
https://github.com/graphql/graphql-js/blob/2aedf25e157d1d1c8fdfeaa4c0d2f3d9d3457dba/src/execution/execute.ts#L1726-L1735
So it's only _iterating over_ the `AsyncIterable` — the "middle" step of execution — where `graphql-js` doesn't catch errors and convert them to `{data: null, errors: ...}` objects.
This seems neither consistent nor desirable, right?
### Alternatives
We can change our code to:
* Have our `AsyncIterable` never throw in `next()` (try/catch every iteration ourselves)
* Have it instead always return a wrapper type, mimicking `{data, errors}`
* Define a `resolve` method just to unwrap this type (even if we have no need for custom resolving otherwise)
* And have this `resolve` method `throw` any `errors` or `return data` if no errors
Doing this would obviously be pretty manual, though, and we'd have to do it for every subscription we have.
### Relation to spec
Given the explicit test case, I wasn't sure at first if this was an intentional implementation/interpretation of the spec.
I'm not clear from reading the spec, and it looks like at least one other person wasn't either: https://github.com/graphql/graphql-spec/issues/995.
But I think my own interpretation is that the spec doesn't explicitly say to re-throw errors. It just doesn't say what to do.
And I believe that `graphql-js` is inconsistent in its handling of errors, as shown above. The spec also doesn't seem to clearly specify how to handle errors creating source event streams, yet `graphql-js` (nicely) handles them.
I hope you'll consider handling errors _iterating over_ source event streams too! Thank you.
Contributor guide
Assessment
This issue has not been assessed yet.