Unsubscribe deadlocks AsyncGenerator

[monitorjbl]

Screenshot
none

Expected Behaviour
When a client wants to cancel a subscription, it sends a "complete" message:

{"id":"fbb454fd-b428-4a5a-82b5-b34bc5c28d9c","type":"complete"}

This is intended to close the AsyncGenerator returned by the resolver's subscribe() function. graphql-ws does this by using the return() method on the AsyncGenerator. In all situations, the AsyncGenerator should actually stop.

Actual Behaviour
If the AsyncGenerator's next() function is being awaited by the subscription when that return() call happens, the generator will not return until the next() call resolves. For all intents and purposes, this is a deadlock that will not resolve unless whatever is blocking next() returns. This means that any implementation that needs to clean up subscriber resources will not do so.

This is a well-known problem with AsyncGenerators in Javascript, even seen before with graphql. There is no fixing this deficiency with AsyncGenerators, as they are operating as (badly) designed. And there is a workaround for it: make your own AsyncIterator implementation that implements the return call directly rather than use the native language feature. It's a pain, but you can see from this example that it works: https://codesandbox.io/p/sandbox/async-generator-vs-iterator-returns-s4sht

IMO, this gotcha should at the very least be documented clearly. Might have missed something, but I didn't see this anywhere in the graphql-ws docs. It might also be a good idea to expose an onUnsubscribe method on useServer()to allow implementers to signal to their AsyncGenerator to reject the awaited promise.

Debug Information
none

Further Information
none

[enisdenjo]

While indeed inconvenient, this is not a design flaw with the AsyncGenerator. This actually mirrors how synchronous generators work - you can't forcibly terminate a generator that's in the middle of executing code. The async version extends this principle to async operations. That is an important safety mechanism!

Let's go over an example where abrutply killing/ending a generator on return while waiting for next is unsafe. Say we have a hypothetical generator that subscribes to over an SSE connection and emits events received:

async function* subscribeToEvents() {
  const eventSource = new EventSource('/events');
  
  try {
    while (true) {
      // in a forever loop to get all events until cancelled
      const event = await waitForNextEvent(eventSource);
      yield event;
    }
  } finally {
    // very important cleanup, must remove listener to avoid memory leaks
    eventSource.close();
  }
}

If this design decision did not exist and our approach was naive: calling a return on the subscribeToEvents generator while waiting for next event would immediately terminate the generator's execution, i.e. the execution would suddently stop on await waitForNextEvent() - never invoking the finally block and never removing the listener leading to a resource leak.

The solution to this issue is to have the await waitForNextEvent() throw when the generator returns (breaking the loop and calling the finally block), which is the current and safe behaviour of async generators. This of course require you to implement your own async generator in order to make the waitForNextEvent aware of the return invokation.

Keeping the example in mind, the same principle applies to next/return, if next is being awaited - you cannot just magically stop it's execution context, it has to run through - you have to "listen" for return in next and break it.

P.S. I personally recommend using RepeaterJS.

[monitorjbl]

The solution to this issue is to have the await waitForNextEvent() throw when the generator returns (breaking the loop and calling the finally block), which is the current and safe behaviour of async generators.

Yeah, I get why it needs to work this way. My particular beef with the design of AsyncGenerators is that none of the native language support lets you do this without opening yourself up to deadlocks. For anything beyond trivial examples, you have to roll your implementation. The async function*() syntax seems unlikely to ever be useful in real world implementations because of this; a better version of that syntax might have embedded an AbortSignal in this that return()/throw() triggers.

AsyncGenerators being this way are not graphql-ws's fault, obviously. But, all the examples I found in the docs use the async function*() syntax, and this was the first time I had to work directly with AsyncGenerators. It worked at first, but when I tried to add the unsubscribe functionality to my client, things went sideways.

Debugging it took about 30 minutes so it wasn't too bad, but it's not an obvious trap until you've already fallen into it once. IMO, the graphql-ws recipes should have at least an example or two of the real-world version most folks would need to use and ideally a word of caution about using any of the toy examples in a real app. Would save others time.

[enisdenjo]

The async function*() syntax seems unlikely to ever be useful in real world implementations because of this

That is very much true!

But, all the examples I found in the docs use the async function*() syntax, and this was the first time I had to work directly with AsyncGenerators

[...]

IMO, the graphql-ws recipes should have at least an example or two of the real-world version most folks would need to use and ideally a word of caution about using any of the toy examples in a real app. Would save others time.

I could find only one, the getting started example that emits the greetings. But I still get your point - I remember my first time working with them, had a few gotchas myself.

Nevertheless, I agree that the documentation could be improved there to get people up-and-running faster. Would you like to lead this and improve the points in documentation you find lacking? I think you would be great for that task given that all of your "gotchas" are fresh. 🙏

[monitorjbl]

Sure, I can get a PR up for this.