Skip to content

Add an option to avoid releasing PubSub connection #275

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from

Conversation

flupke
Copy link
Contributor

@flupke flupke commented Aug 30, 2012

I ran into a problem trying to reuse a single PubSub object for many channels:

client = Redis()
pubsub = client.pubsub()
pubsub.subscribe('foo')
# [...]
pubsub.unsubscribe('foo')
pubsub.subscribe('bar')
for message in pubsub.listen():
    pass

The last listen() fails, because the PubSub connection is None at this time:

Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/gevent/greenlet.py", line 328, in run
    result = self._run(*self.args, **self.kwargs)
# [...]
  File "/home/flupke/src/ext/redis-py/redis/client.py", line 1496, in listen
    r = self.parse_response()
  File "/home/flupke/src/ext/redis-py/redis/client.py", line 1440, in parse_response
    response = self.connection.read_response()
AttributeError: 'NoneType' object has no attribute 'read_response'

This is because I wasn't parsing responses, and PubSub recycles the connection when it catches a response indicating there are no more channels subscriptions. The fix is to parse responses after each pub/sub call:

client = Redis()
pubsub = client.pubsub()
pubsub.subscribe('foo')
pubsub.parse_response()
# [...]
pubsub.unsubscribe('foo')
pubsub.parse_response()
pubsub.subscribe('bar')
pubsub.parse_response()
for message in pubsub.listen():
    pass

This is a bit cumbersome, but the real problem is that an unnecessary Redis disconnect/connect cycle is made. The patch adds a release_connection option to StrictRedis.pubsub() and PubSub's constructor to avoid this behavior.

@travisbot
Copy link

This pull request passes (merged 2ef7730 into 5209844).

@leetrout
Copy link

leetrout commented Feb 5, 2014

Is there a reason this hasn't been merged?

I've hit the same issue...

andymccurdy pushed a commit that referenced this pull request Mar 13, 2014
…release the connection when all channels are unsubscribed. fixes #284 and #275
@andymccurdy
Copy link
Contributor

Hi. Many apologies on taking so long to cleanup Publish/Subscribe in redis-py. I've recently had some time to make some big improvements and I'd love to get your feedback.

I believe that the changes I've made solve this issue.

You can review the changes in the pubsub branch here: https://github.com/andymccurdy/redis-py/tree/pubsub

I've written a section in the README docs about PubSub that covers most (hopefully all!) use cases. You can find it here: https://github.com/andymccurdy/redis-py/blob/pubsub/README.rst#publish--subscribe

My current plan is to merge the pubsub branch into master and roll a new release of redis-py by the end of the week. Again, I'd love any feedback.

@andymccurdy
Copy link
Contributor

redis-py 2.10 is out with the new PubSub system.

@andymccurdy andymccurdy closed this Jun 2, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants