graphql-subscriptions: iterable.return() is not sufficient to clean up subscriptions

@jquense Okay let me clarify, cause yeah this stuff is not obvious.

In for await (let val of foo), foo must be an AsyncIterable, but not necessarily AsyncIterator. When the loop starts it calls foo[Symbol.asyncIterator] to create the actual AsyncIterator, which can be a completely different object than foo (and ought to be a different object in most use cases). The loop then repeatedly awaits next() on the iterator until it is done, in which case it calls the iterator’s return() method, or an error is thrown, in which case it calls the iterator’s throw() method.

Here’s the consequence: if the for await loop never runs (like in your example) it never calls the [Symbol.asyncIterator] method. Which means if you only perform listener registration when [Symbol.asyncIterator]() runs, you won’t register listeners unless the loop is actually going to run (in which case you’re guaranteed to get a return() or throw() call in which you can unregister the listeners).

So here’s a safe way to rewrite your original example:

const iterable = {
  [Symbol.asyncIterator]() {
    let i = 0
    console.log('adding listener!')
    // below is the actual iterator -- this creates a new iterator with each call
    return {
      next() {
        return Promise.resolve({ value: i++, done:  i >= 4 })
      },
      return() {
        console.log('removing listener!')
        return Promise.resolve({ value: undefined, done: true });
      },
      throw(error) {
        console.log('removing listener!')
        return Promise.reject(error);
      },
    }
  }
}

async function* addOne(iterable) {
  for await (const val of iterable) yield val + 1;
}

Now if I call the generator but make its iterator return() before I ever call next(), it never registers the “listener”:

> let iterator = addOne(iterable)
undefined

> await iterator.return()
{value: undefined, done: true}

However, the first time I call next(), it actually runs the for await statement, and the first thing that does is call iterable[Symbol.asyncIterator](), which is what actually adds the listener. And when I call return() in the middle of the for await, it does pass through the return call, removing the listener:

> iterator = addOne(iterable)
addOne {<suspended>}

> await iterator.next()
adding listener!
{value: 1, done: false}

> await iterator.return()
removing listener!
{value: undefined, done: true}

@TimSusa This is how we ended up handling things, BTW: https://github.com/4Catalyzer/graphql-subscription-server/pull/20. We still expose an async iterable, because they’re nice to work with for various operations, but we also thread through a separate “close” method that we use to clean things up.

@taion I understand you are frustrated and would like to see a sea change here, and I agree that we need to figure out a better solution for this, but please be technically honest: iterator.return() will work as long as people don’t use async generators. “Just not going to work” will mislead people into thinking it will not work in any case.

I think it’s important to be clear about this because solving this issue by switching to observables will be a far-reaching change requiring coordination between many different parts of the ecosystem and change in everyone’s application code, not necessarily something I’m opposed to, but if it even happens it will take a lot of time. So until that day, it’s important that we spread the word that people should avoid using async generators for GraphQL subscriptions and try to clearly explain why.

From the other thread, this is much worse than previously understood given https://github.com/tc39/proposal-async-iteration/issues/126#issuecomment-403454433.

At the current point, iterator.return() is just not going to work for tearing down and cleaning up resources.

It’s worth keeping in mind FWIW that the issue isn’t generator functions per se. It’s about adapting external event sources (something like an observable) to an async iterable.

Right now these options all seem pretty mediocre.

I don’t believe this is a problem for subscriptions Apollo because I don’t think it uses for await anywhere under the hood.

Looking at subscriptions-transport-ws, I believe it calls an AsyncIterator’s return method here: https://github.com/apollographql/subscriptions-transport-ws/blob/master/src/server.ts#L211

But keep in mind: no AsyncIterables anyone is using for GraphQL subscriptions should do anything that necessitates cleanup until their Symbol.asyncIterator method is called. As long as they follow that contract then they will work safely in for await statements without leaving any dangling resources.