Skip to content
This repository has been archived by the owner on Apr 3, 2019. It is now read-only.

Ensure that there are no subscribers if there is no Redis connection #73

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

hagsteel
Copy link

I experienced issues if my Redis server was restarted: the list of subscribers didn't change while there was no subscription to a channel in Redis.

By resetting the subscribers if there is no connection we ensure that there will be a subscription to a channel in Redis (rather than piling up subscribers that will never receive a message).

(Not all tests were passing when I ran the tests before I applied this so I don't think this will break anything)

# If for some reason the redis client is not connected (the server might have gone down)
# then clear the subscribers. This ensure that there are no subscribers since there possibly can't be an
# active Redis subscription
if not self.redis.connection.connected():
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this won't solve the issue.
I think we need to add a "reconnect" event handler, sending "subscribe/psubscribe" messages for each handled channel.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you are absolutely right, this was more of a quick fix (before I applied this to my fork it stopped responding after the server had been running for a while, but with this it doesn't).

I would be more than happy to throw some ideas around.

@AeroNotix
Copy link

These kinds of bugs are somewhat concerning for the state of the library. The loss of connection and handling these cases is fundamental to a library such as this.

I believe some kind of test suite which stresses the library in the face of connection loss needs to be implemented.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants