fix: ensure asyncIterator call return() when fail at calling next()

[niceandneat]

This calls .return() of asyncIterator before remove it from subscriptions.

After microsoft/TypeScript#51297, error at iterator.next() in for await (... of iterator) statement does not call iterator.return(). This can cause asyncIterator not to be cleared and hang.

[enisdenjo]

Hey thanks for this! I am just wondering why calling .return is necessary here. If you throw from a running iterator it is destroyed anyways, a .return call would be superfluous - right?

[niceandneat]

@enisdenjo

Nice point! I thought it would be necessary cause some custom async iterator implementations contain clear logic at .return(), or .throw() (like this one). But if an error throws at .next() call during for await loop, neither .return(), nor .throw() runs. So it can make some external resources remain in the memory and not free them.

It can be seen as the responsibility of implementations, but this problem can be prevented by the consumer of async iterator and this PR is about that prevention.

[enisdenjo]

I understand your point, but beware that neither .return or .throw are callbacks - they are methods used for controlling the iterator itself and are therefore not a good place for side-effects. Meaning, the convention of using .return for cleanups is actually off-putting.

As per MDN:

• .throw method of a generator acts as if a throw statement is inserted in the generator's body at the current suspended position
• .return method of a generator acts as if a return statement is inserted in the generator's body at the current suspended position

And then .return method documentation on MDN continues:

...which finishes the generator and allows the generator to perform any cleanup tasks when combined with a try...finally block

MDN about .return

Hinting that cleanups are to be done in the finally block of a try-catch statement.

[niceandneat]

I understand your point, but beware that neither .return or .throw are callbacks - they are methods used for controlling the iterator itself and are therefore not a good place for side-effects. Meaning, the convention of using .return for cleanups is actually off-putting.

Yes, I also think .return() should not be used for cleanups. Actually this is about restoring the old behavior.

graphql-ws/src/server.ts

Lines 847 to 854 in 611c223

	return async (code, reason) => {
	if (connectionInitWait) clearTimeout(connectionInitWait);
	for (const sub of Object.values(ctx.subscriptions)) {
	if (isAsyncGenerator(sub)) await sub.return(undefined);
	}
	if (ctx.acknowledged) await onDisconnect?.(ctx, code, reason);
	await onClose?.(ctx, code, reason);
	};

graphql-ws/src/use/ws.ts

Lines 137 to 151 in 611c223

	try {
	await cb(String(event));
	} catch (err) {
	console.error(
	'Internal error occurred during message handling. ' +
	'Please check your implementation.',
	err,
	);
	socket.close(
	CloseCode.InternalServerError,
	isProd
	? 'Internal server error'
	: limitCloseReason(err.message, 'Internal server error'),
	);
	}

Before 611c223, .return() of remain async iterators (includes thrown one) was called in the return function of .opened() at situations like early exits(e.g. throw at .next()), because socket closes at catch block of .opened() call.

After that commit, thrown async iterator is removed from ctx.subscriptions and its .return() cannot be called. But other async iterators that are contained in the same subscriptions call .return() same as before.

[enisdenjo]

We only call .return on async iterators if we're the ones invoking the cleanup (connection closes or user-triggered completes), otherwise we should not.

I don't fully follow your last comment, how's this behavior different with 611c223?

A further flaw with this PR is also that .return will be called twice when the generator completes, this would definitely be breaking.

We must assume that if the async generator read loop exits/throws - its due to user intervention (either by calling .return or .throw or throwing in .next).

[niceandneat]

I don't fully follow your last comment, how's this behavior different with 611c223?

Sorry for confusing. In short,

Before 611c223

If throwing in .next happens at one async iterator, every async iterators in the same ctx.subscriptions call .return includes thrown async iterator.

After 611c223

If throwing in .next happens at one async iterator, every async iterators in the same ctx.subscriptions call .return excepts thrown async iterator. Because thrown async iterator removed from ctx.subscriptions before socket close callback is called.

A further flaw with this PR is also that .return will be called twice when the generator completes, this would definitely be breaking.

That's the side effect I missed, thanks for checking! I think this PR should be closed because of the flaw above 😅.

[enisdenjo]

After 611c223

If throwing in .next happens at one async iterator, every async iterators in the same ctx.subscriptions call .return excepts thrown async iterator. Because thrown async iterator removed from ctx.subscriptions before socket close callback is called.

And this is how it should be. 😄

[niceandneat]

Yep I get it. Thanks for answering!