Return function of subscription's async iterator never resolves on some versions of Node 14

[christhegrand]

I'm trying to upgrade my codebase from Node 12.21 to Node 14, and I noticed that on some versions of Node 14, the return() function called in subscription/mapAsyncIterator.js never actually resolves the promise it returns, so the code that we have in a .then() never runs. I've tried to put together a test case for this, but frustratingly enough I can't seem to reproduce this in a short test script, although we have a couple of tests that can reproduce this problem every time.

Anyway, we create a subclass of a passthrough stream like this:

export class MessageStream extends PassThrough {
  constructor(opts = {}) {
    opts.objectMode = true
    super(opts)
  }
  write(msg, encodingOrCB, cb) {
    return super.write(msg, encodingOrCB, cb)
  }
}

Our subscribe function returns this message stream, which the GraphQL library then creates an async iterator from:

            subscribe: (parent, arg, ctx) => {
                const messageStream = new MessageStream();
                let unsubFuncs = [];
                let closed = false;
                function close() {
                    for (const unsub of unsubFuncs) {
                        unsub();
                    }
                }
                args.subscribe(parent, arg, ctx, messageStream).then(unsubs => {
                    unsubFuncs = unsubs;
                    // this is to fix a potential race condition when the stream gets closed before the promise resolves
                    if (closed) {
                        close();
                    }
                }).catch((e) => {
                    server_config_1.default.warn(`error on subscription ${e.message} ${e.stack}`);
                    messageStream.end();
                });
                // need to have both close and end
                // close is for when the client closes
                // end is for when we end the stream like on an error
                // or from within a resolver
                messageStream.on('close', () => {
                    console.log('in close event');
                    close();
                    closed = true;
                });
                messageStream.on('end', () => {
                    close();
                    closed = true;
                });
                messageStream.on('error', (e) => {
                    server_config_1.default.warn(`error on subscription ${e.message} ${e.stack}`);
                });
                return messageStream;
            }
        };
    };
};

We then store the async iterator returned from the GraphQL subscription for later use. When a user unsubscribes, we call the return function on the async iterator, and have some code that runs in a .then afterwards. This is where we are seeing a problem. On Node 12.21, which we were previously using, the code in the .then() runs just fine, and our send function gets called:

           ai.return().then(_ => {
              send({id, data: {unsubscribe: true}}, req)
            }).catch(err => {
              console.log(err)
            })

But on some versions of Node 14, the code in the .then and .catch never runs at all, because the promise returned by the return() function never actually resolves. The code above works in Node 14 up to Node 14.10.0, when it breaks, then it works in Node 14.10.1, then it breaks again in Node 14.14.0 and higher. The change that seems to be breaking the code is a change to the async iterator code in Node 14, which originally went out in Node 14.10.0, was reverted in 14.10.1, and which was then re-added in Node 14.14.0. You can see the change here: nodejs/node@573410f

There's some discussion of the change here too: nodejs/node#34887

Anyway, I hope this is all clear enough... I see that the $return variable that gets set in subscription/mapAsyncIterator.js is using iterator.return, which looks like it's a different function in the latest versions of Node 14 (you can see this in the deleted async iterator file's diff above). Does this return function now need to be pointing to a different function in the underlying iterator in order to work as expected?

[yaacovCR]

I’m afraid I couldn’t follow the node internals. It does not seem that the above node change has a public API change correlating.

It’s difficult to assess further unfortunately without a reproducible example…

is there any status update from your end?

[christhegrand]

We could never figure out how to make this work with streams on later versions of Node, and rewrote our code to use an async iterator instead.