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

Make sure free_client() method is called on connection retries. #57

Merged
merged 1 commit into from
Oct 15, 2013
Merged

Make sure free_client() method is called on connection retries. #57

merged 1 commit into from
Oct 15, 2013

Conversation

arohter
Copy link
Contributor

@arohter arohter commented Oct 15, 2013

We need to explicitly call free_client() within the retry loop, as the ensure section is called only once, on rescue block exit.

Every time a redis command is sent, client_for() is called, adding the client connection object to a stack. Under normal usage, a subsequent free_client() call is made, removing this client object from the stack. However, on connection failure, with retries enabled (the default), the rescue::retry loop triggers client_for() multiple times, without matching free_client() calls (ruby ensure sections are only called at final rescue block exit). This results in duplicate copies of the client object to remain in stack, which effectively caches the client connection forever, resulting in perpetual stale server selection, breaking failover completely.

We ran into this almost immediately when testing failure handling with a simple rpush+blpop ping/pong test.
I'm pretty sure #42 is a manifestation of the same problem, and why using .clear instead of .pop happened to "fix" things for them.

We need to explicitly call free_client within the retry loop, as the ensure section is only called once, on resque block exit.
@ryanlecompte
Copy link
Owner

Thanks for catching this, @arohter. Merging.

ryanlecompte added a commit that referenced this pull request Oct 15, 2013
Make sure free_client() method is called on connection retries.
@ryanlecompte ryanlecompte merged commit 15778c7 into ryanlecompte:master Oct 15, 2013
@arohter arohter deleted the free_client-retry-fix branch October 15, 2013 21:23
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.

2 participants