search

apollographql / graphql-subscriptions

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

1.1 pubsub next() not being resolved if publish called first #200

Closed acoreyj closed 5 years ago

acoreyj commented 5 years ago

I believe this probably has to do with the latest changed to pubsub and asynciterator, as my code and tests work with 1.0 but not 1.1

Edit - Showing the failure in codesandbox

1.1 next() never resolves - https://codesandbox.io/s/j4022nnm3w?fontsize=14 1.0 next()resolves - https://codesandbox.io/s/oxorqwyl59?fontsize=14 1.1 next()resolves if publishcalled after next() - https://codesandbox.io/s/w70lo2qpkk?fontsize=14

My code:

Essentially this takes a pubsub and adds subscription types and resolvers and sets up the publish:

https://github.com/genie-team/graphql-genie/blob/master/plugins/subscriptions/src/subscriptions.ts

Here is the test that is now failing

https://github.com/genie-team/graphql-genie/blob/master/plugins/subscriptions/tests/__tests__/subscriptions.test.ts

The AsyncIterator returned from graphql subscribe never has next resolved, even though publish is called on the pubsub

Test results in timeouts

https://circleci.com/gh/genie-team/graphql-genie/92?utm_campaign=build-failed&utm_medium=email&utm_source=notification

I even put a breakpoint in the withfilter method, next is not being called there either.

grantwwu commented 5 years ago

I'm on vacation right now, will try to look at this on Monday

acoreyj commented 5 years ago

Thanks, updated with very basic example on codesandbox demonstrating next() not resolving, maybe I'm misunderstanding something otherwise there is something going wrong with the pubsub implementation

acoreyj commented 5 years ago

Seems like next() resolves if the publish is called after next() is called. But the subscription should already be done by calling .asyncIterator in both cases I believe.

grantwwu commented 5 years ago

Thanks for doing some digging. It seems that this is a pretty clear consequence of https://github.com/apollographql/graphql-subscriptions/commit/c8a7f7cf4897c479d15631c9a9039dba93054a80

This change was made to avoid certain memory leaks; it's not clear to me if there's any alternative given how AsyncIterators work.

Is it possible to change your code to always call next() before calling publish?

acoreyj commented 5 years ago

Ah, yup that is possible. Seems like the old behavior was the wrong behavior and the new behavior makes more sense. Thanks