Skip to content

Commit

Permalink
We need to be more pro-active with client management, since node-redis
Browse files Browse the repository at this point in the history
will never emit an end event when calling client.end() (crazy, right?).
Also add a concept of unmanaged connections, and test it.
  • Loading branch information
Andrew Houghton committed Mar 5, 2015
1 parent 296e2ab commit f8ebce9
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 10 deletions.
20 changes: 20 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,26 @@ var sentinelClient = sentinel.createClient(endpoints, {role: 'sentinel'});

Where you should also transparently get a reconnection to a new slave/sentinel if the existing one goes down.

## Connection Management ##

By default node-redis-sentinel will attempt to reconnect any existing redis clients when a failover is
detected. In some cases you'll want to manage your own connection lifetime and tell node-redis-sentinel
not to maintain a reference to the redis client -- you can pass explicitly set the 'sentinel_managed'
option to false when creating a client:

```javascript
var sentinel = require('redis-sentinel');

var endpoints = [
{host: '127.0.0.1', port: 26379},
{host: '127.0.0.1', port: 26380}
];
var masterName = 'mymaster';

var Sentinel = sentinel.Sentinel(endpoints);
var masterClient = Sentinel.createClient(masterName, {sentinel_managed: false});
```

## TODO ##
* We could probably be cleverer with reconnects etc. and there may be issues with the error handling

Expand Down
29 changes: 19 additions & 10 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ Sentinel.prototype.createClient = function(masterName, opts) {
self.pubsub.push(pubsubClient);
}
return this.createClientInternal(masterName, opts);
}
};

Sentinel.prototype.createClientInternal = function(masterName, opts) {
if (typeof masterName !== 'string') {
Expand All @@ -56,25 +56,34 @@ Sentinel.prototype.createClientInternal = function(masterName, opts) {

opts = opts || {};
var role = opts.role || 'master';
var sentinel_managed = opts.sentinel_managed !== false;
delete opts['sentinel_managed'];

var endpoints = this.endpoints;


var netClient = new net.Socket();
var client = new redis.RedisClient(netClient, opts);
this.clients.push(client);

var self = this;

client.on('end', function() {
// if we're purposefully ending, forget us
if (this.closing) {
var index = self.clients.indexOf(this);
if (index !== -1) {
self.clients.splice(index, 1);
if (sentinel_managed) {
this.clients.push(client);
client.on('end', function() {
// if we're purposefully ending, forget us
if (this.closing) {
var index = self.clients.indexOf(this);
if (index !== -1) {
self.clients.splice(index, 1);
}
}
}
});
});
}

// clients who call end() won't trigger an end event (node-redis forcibly clears the event
// observers from the underlying stream on client.end()); we need to regularly filter for
// closing connections, and this is a reasonable place
this.clients = this.clients.filter(function(client) { return !client.closing; });

function connectClient(resolver) {
return function(err, host, port) {
Expand Down
27 changes: 27 additions & 0 deletions test/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -223,5 +223,32 @@ describe('Redis Sentinel tests', function() {
});
});
});

it('should not manage unmanaged clients', function (done) {
var endpoints = [{host: '127.0.0.1', port: 26380}];
var instance = sentinel.Sentinel(endpoints);
var redisClient1 = instance.createClient('mymaster');
redisClient1.on('ready', function () {
// one pubsub, one actual
expect(instance.clients.length).to.equal(2);

var redisClient2 = instance.createClient('mymaster', {sentinel_managed: false});
redisClient2.on('ready', function () {
expect(instance.clients.length).to.equal(2);

redisClient1.info(function(err, info) {
expect(err).to.be.null;
expect(info).to.be.ok;

redisClient2.info(function(err, info) {
expect(err).to.be.null;
expect(info).to.be.ok;

done();
});
});
});
});
});
});
});

0 comments on commit f8ebce9

Please sign in to comment.