Sever should probably handle async iterator with no 'return' somehow

[loganfsmyth]

Mostly a driveby bug as I was skimming the code. Hope this is clear and not too nitpicky and I didn't missing anything.

I was looking at

graphql-ws/src/server.ts

Line 699 in 6ab96d1

for await (const result of operationResult) {

and noticed that this loop will run forever unless the producer decides to stop producing, but the return at

graphql-ws/src/server.ts

Line 718 in 6ab96d1

await ctx.subscriptions[message.id]?.return?.();

is called with ?.(), so if it doesn't exist, the for await will never exit and the client will continue to get events.

It looks like graphql does have a return, but since useServer accepts a custom subscribe/execute function, you never know exactly what you'll get, even if you document it as "pass me exactly a callback that does exactly what graphql.subscribe does. It seems to me like either return should to be required and throws a JS error if it's missing, or the whole for-await should also be wrapped with a race against a promise resolved at MessageType.Complete and the loop should check if it was unsubscribed on every iteration.

[n1ru4l]

Actually, every execute or subscribe function should implement the AsyncIterableIterator interface and have a return property that can be called. I guess the ?.() call is just there to please the TypeScript compiler.

An alternative that might trigger a runtime error in case the execute or subscribe function is malfunctioning and does not have a callable return property would be to use the ! operator. e.g. await ctx.subscriptions[message.id]!.return!();

However, most people won't use a custom execute or subscribe function anyways, and if they do the library maintainers probably would already have taken care of it because they know they do something very specific?

Maybe the graphql-js execute and subscribe functions could instead use not the AsyncIterableIterator return type but instead, a custom one that has a mandatory return property?

[loganfsmyth]

I guess the ?.() call is just there to please the TypeScript compiler.

Yeah that was my assumption, At a minimum I guess it should be ?.return!() so it'll throw if it is missing.

Maybe the graphql-js execute and subscribe functions could instead use not the AsyncIterableIterator return type but instead, a custom one that has a mandatory return property?

That seems like the easiest approach.

[enisdenjo]

Hey @loganfsmyth, not nitpicky at all, I like the insight! Thanks to @n1ru4l for answering and correctly assuming ?.() being there to satisfy the TS compiler.

However, you guys are right, non-null assertion is a better fit because the return method MUST be implemented for having expected behaviour. Fix is already on master.

[enisdenjo]

🎉 This issue has been resolved in version 4.3.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀