search

davidyaha / graphql-redis-subscriptions

A graphql subscriptions implementation using redis and apollo's graphql-subscriptions
MIT License
1.11k stars 125 forks source link

Fix concurrent subscribe race condition #599

Closed mlaflamm closed 4 months ago

mlaflamm commented 1 year ago

Fix for #598

I created a unit test that reproduces the scenario described in the issue. This is unfortunately a convoluted test. Only the subscribe/unsubscribe flow is exercised. No message delivery is attempted.

As for the fix, we keep a pending state per channel, subsPendingRefsMap, before a new remote subscribe call is made to redis. That state is removed once the redis call is completed. The state keeps track of the subscription initiated while the redis call is in flight. The state also keeps a reference to the initial subscription promise.

I had to use a deferred promise in the state. :crying_cat_face: The state must be initialized before executing the promise but the execution is done in the promise constructor.

elibosley commented 1 year ago

This is a very bad bug, and this PR seems to resolve the issues we were having with it. Would love if @davidyaha could get this merged.

davidyaha commented 1 year ago

Hey @mlaflamm Thanks for catching and fixing this! Sorry for my absence in the past months, I would like to review and merge this but I can't seem to find the time.

I've added @elibosley as a reviewer (it only let me put him here as he is the only commenter) if 1 more person other than @elibosley (maybe @jstaro) could review, I would feel more comfortable with merging and releasing a new version.

Does that work for everyone involved?

davidyaha commented 1 year ago

BTW, I do think this might be able to be resolved on the using library but I think this lib should also have safety measures to such scenarios as it is kind of a pain to debug and find the issue. so again thanks @mlaflamm for going after it!

puchm commented 8 months ago

Since this has been approved by everyone who needed to, can we merge this and push out an update?

shenma6 commented 5 months ago

can we merge this one?

shenma6 commented 5 months ago

@davidyaha can you merge it ?

shenma6 commented 4 months ago

@elibosley @bradens @jainshripal can you merge this fix in?

pionxzh commented 4 months ago

Hi, can we ship this? thanks 🙏

davidyaha commented 4 months ago

Thanks everyone! Sorry again for my absence.