search

apollographql / graphql-subscriptions

:newspaper: A small module that implements GraphQL subscriptions for Node.js
MIT License
1.59k stars 133 forks source link

create separate pubsub.asyncIterable method and deprecate pubsub.asyncIterator #147

Closed jedwards1211 closed 2 years ago

jedwards1211 commented 6 years ago

As @jquense wisely helped me realize in #143, pubsub.asyncIterator is unsafe and could leave dangling event listeners when consumed by 3rd-party code.

In general the documentation mistakenly states that GraphQL subscribe resolvers must return an AsyncIterator; rather, they should return an AsyncIterable, and event listener registration/anything else necessitating cleanup should only be performed when that AsyncIterable's Symbol.asyncIterator method is actually called, since for await and probably most 3rd-party code is only guaranteed to return the iterator if it ends up starting iteration (i.e. calling Symbol.asyncIterator).

As a solution, I've created a new pubsub.asyncIterable method that we should advise people to use. I also made eventEmitterToAsyncIterator print a deprecation warning when it's used like an AsyncIterable (which will happen with the current recommended usage of pubsub.asyncIterator).

The withFilter method still needs to be redesigned to operate on AsyncIterables instead of AsyncIterators.

jedwards1211 commented 6 years ago

I have refactored the withFilter method to operate on AsyncIterables rather than AsyncIterators.

jedwards1211 commented 6 years ago

@jonbudd actually I don't think this is a work in progress anymore...

jedwards1211 commented 6 years ago

Although I think asyncIterable is a better API design, I was wrong that it can be totally safe.

taion commented 6 years ago

so IMO async iterators still give you quite a nice way to manipulate these things

i think the best way to go is to just have some explicit closeability concept; see what we did in https://github.com/4Catalyzer/graphql-subscription-server/pull/20/files

grantwwu commented 6 years ago

@taion But who calls that? Don't we depend on https://github.com/apollographql/subscriptions-transport-ws to call return right now? Would we need to modify that?

taion commented 6 years ago

the approach we use is:

  1. subscribers return an async iterable and a close callback
  2. we keep track of the close callbacks to call associated with each client subscription
  3. we call all those close methods when we tear down the client subscription

so we just don't use return at all

grantwwu commented 6 years ago

Okay, so that would require modifying the transport...

hwillson commented 2 years ago

Thanks @jedwards1211 and sorry we didn't get to this sooner. A lot has changed with the codebase since this was opened, and some of these changes are already coming in 3.0. I'll close this off but please open any new PR's (ideally based off of the release-3.0 branch) that you would like to see added. @brainkim is helping maintain this project now as well, so the great conversation you both had in https://github.com/tc39/proposal-async-iteration/issues/126 can continue in this repo.

Side note @brainkim - karma:

By the way, two issues/antipatterns I see from lurking in the GraphQL community related to lazy async iterators, but which I just haven’t really commented on because it might seem too self-promotional and I don’t really use GraphQL ...

😂