Skip to content
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

Fix channel loss on reconnect #27

Merged
merged 1 commit into from
Jan 31, 2015
Merged

Fix channel loss on reconnect #27

merged 1 commit into from
Jan 31, 2015

Conversation

kichik
Copy link
Contributor

@kichik kichik commented Jan 31, 2015

The scenario requires two or more close-by disconnections

  1. Connect to Redis
  2. Subscribe successfully to a channel
  3. Forcibly kill the connection (by taking down Redis for example)
  4. Let lettuce reconnect successfully
  5. Forcibly kill the connection again right before activate() successfully subscribes
  6. Let lettuce reconnect successfully
  7. RedisPubSubConnectionImpl.channels is empty and the channel will stay unsubscribed

There doesn't seem to be a reason to clear out channels and patterns.
It is only used for persistence over different connections.
There is no downside to having a channel in that set even if the subscription was not successful.

The scenario requires two or more close-by disconnections
1. Connect to Redis
2. Subscribe successfully to a channel
3. Forcibly kill the connection (by taking down Redis for example)
4. Let lettuce reconnect successfully
5. Forcibly kill the connection again right before activate() successfully subscribes
6. Let lettuce reconnect successfully
7. RedisPubSubConnectionImpl.channels is empty and the channel will stay unsubscribed

There doesn't seem to be a reason to clear out channels and patterns.
It is only used for persistence over different connections.
There is no downside to having a channel in that set even if the subscription was not successful.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 96.29% when pulling f6b869a on kichik:master into cc638d4 on mp911de:master.

@mp911de
Copy link
Collaborator

mp911de commented Jan 31, 2015

Looks good to me. Both collections patterns and channels are sets. A redis reply of subscribed to the pattern/channel won't cause duplicates. Thank you for the PR.

mp911de added a commit that referenced this pull request Jan 31, 2015
Fix channel loss on reconnect
@mp911de mp911de merged commit b15acc7 into redis:master Jan 31, 2015
@kichik
Copy link
Contributor Author

kichik commented Jan 31, 2015

Thanks for the super fast response (again)!
Any chance you can release 3.0.3 with this?

@mp911de
Copy link
Collaborator

mp911de commented Jan 31, 2015

Will do. Could you check the memory leak fix I pushed earlier this day?

Von meinem iPhone gesendet

Am 31.01.2015 um 20:02 schrieb kichik notifications@github.com:

Thanks for the super fast response (again)!
Any chance you can release 3.0.3 with this?


Reply to this email directly or view it on GitHub.

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.

3 participants