graphql / graphql/graphql-js

Errors thrown when iterating over subscription source event streams (AsyncIterables) should be caught

Open
#4,001 25 comments 8 reactions 0 assignees View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.