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

Close the redis connection on error #49

Merged
merged 1 commit into from Feb 26, 2014
Merged

Conversation

@PaulMougel
Copy link
Contributor

PaulMougel commented Dec 29, 2013

Before losing the reference to this.client, forcibly close the
connection to the Redis server.

If the Redis server couldn’t be reached in the first place
(ECONNREFUSED on connection), this also prevents node-redis from
auto-reconnecting indefinitely.


I couldn't come up with a simple test case, but here is a meaningful use case:

When the Redis server isn't available, client.stop() doesn't work as expected, as something keeps the event loop busy:

var Catbox = require('./');

var client = new Catbox.Client({
    engine: 'redis',
    partition: 'examples'
});

client.start(function (err) {
    console.log(err);
    client.stop();
});

outputs:

~/tmp ❯❯❯ node test.js
[Error: Redis connection to 127.0.0.1:6379 failed - connect ECONNREFUSED]

(the program hangs there forever and needs to be stopped using ^C)

Whereas if the Redis server is available:

~/tmp ❯❯❯ node test.js
undefined
~/tmp ❯❯❯ 
Before losing the reference to this.client, forcibly close the
connection to the Redis server.

If the Redis server couldn’t be reached in the first place
(ECONNREFUSED on connection), this also prevents node-redis from
auto-reconnecting indefinitely.
@LoicMahieu

This comment has been minimized.

Copy link

LoicMahieu commented Jan 9, 2014

+1

@hueniverse hueniverse added the bug label Feb 26, 2014
@hueniverse hueniverse added this to the 1.2.0 milestone Feb 26, 2014
@hueniverse hueniverse self-assigned this Feb 26, 2014
hueniverse added a commit that referenced this pull request Feb 26, 2014
Close the redis connection on error
@hueniverse hueniverse merged commit 3713f24 into hapijs:master Feb 26, 2014
1 check failed
1 check failed
default The Travis CI build failed
Details
hueniverse added a commit that referenced this pull request Feb 26, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.