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

[aseemk]

Context

Hi there. We're using graphql-js and serving subscriptions over WebSocket via 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'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:

graphql-js/src/execution/mapAsyncIterable.ts

Lines 38 to 40 in 2aedf25

	async next() {
	return mapResult(await iterator.next());
	},

There's even a test case today that explicitly expects these errors to be re-thrown:

graphql-js/src/execution/__tests__/subscribe-test.ts

Lines 1043 to 1047 in 8a95335

	it('should pass through error thrown in source event stream', async () => {
	async function* generateMessages() {
	yield 'Hello';
	throw new Error('test error');
	}

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 (enisdenjo/graphql-ws#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:

  graphql-js/src/execution/execute.ts

  Lines 1784 to 1793 in 2aedf25

  	try {
  	const eventStream = executeSubscription(exeContext);
  	if (isPromise(eventStream)) {
  	return eventStream.then(undefined, (error) => ({ errors: [error] }));
  	}
  	return eventStream;
  	} catch (error) {
  	return { errors: [error] };
  	}

• 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:

  graphql-js/src/execution/execute.ts

  Lines 1726 to 1735 in 2aedf25

  	return mapAsyncIterable(
  	resultOrStream,
  	(payload: unknown) =>
  	executeImpl(
  	buildPerEventExecutionContext(exeContext, payload),
  	// typecast to ExecutionResult, not possible to return
  	// ExperimentalIncrementalExecutionResults when
  	// exeContext.operation is 'subscription'.
  	) as ExecutionResult,
  	);

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: graphql/graphql-spec#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.

[yaacovCR]

I agree that the spec is agnostic, and that it would be useful for graphql-js to be consistent and provide explanatory errors. I think the spec should also be improved.

For context, it seems from #918 that prior to that PR, all subscribe errors threw, and that the argument was made there that explanatory errors would be helpful in some cases. The parts of the PR that I skimmed through doesn't seem to indicate why explanatory errors to the client would not be helpful with iteration errors; my suspicion is that the PR was attacking the low-hanging fruit, and the authors/reviewers there would not necessarily object to even more explanatory errors. :)

@robzhu @leebyron

[yaacovCR]

I think the next step would be to raise this topic at a working group meeting. @aseemk are you interested in championing this there? (I am potentially dangerously assuming that this hasn’t happened already…)