RedisStore can have its users (e.g connect-session) handle connection to Redis problems #54

Merged
merged 5 commits into from Sep 5, 2012

2 participants

@louischatriot

Hello TJ,

As discussed here, I implemented a "safe mode" for the RedisStore. If enabled through the "safeMode" option, the 'error' event emitted by the redis driver is caught, preventing a node crash, and a 'connectionError' is emitted instead, which the user can use to know what state the RedisStore is in and act accordingly. In the case of Connect's session middleware, if the RedisStore is not ready, we simply pass the request to the next middleware, thus acting as if there was no session at all.

Related PR on connect : PR #653

Louis

@tj tj commented on an outdated diff Aug 28, 2012
lib/connect-redis.js
@@ -66,10 +67,17 @@ module.exports = function(connect){
self.client.send_anyways = false;
});
}
+
+ // Let users of the RedisStore handle connection to Redis issues if safe mode enabled
+ if (options.safeMode) {
+ self.client.on('error', function () { self.emit('connectionError'); });
@tj
Owner
tj added a line comment Aug 28, 2012

we could get away with removing the safeMode option I think, since emitting these events wont really harm anything unless someone is listening on them. Also a small nitpick but I would prefer emitting "disconnect" and "reconnect", something like that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@louischatriot

Indeed, the safeMode option was not a must-have, old habits die hard :) I made the changes to this PR and the reflected them in the PR on Connect.

@tj tj commented on an outdated diff Sep 3, 2012
lib/connect-redis.js
@@ -66,10 +67,15 @@ module.exports = function(connect){
self.client.send_anyways = false;
});
}
+
+ // Let users of the RedisStore handle connection to Redis issues if safe mode enabled
+ self.client.on('error', function () { self.emit('disconnect'); });
+ self.client.on('connect', function () { self.emit('reconnect'); });
@tj
Owner
tj added a line comment Sep 3, 2012

only tiny change is that I renamed this to connect in Connect to match node's names for these events

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tj tj commented on an outdated diff Sep 3, 2012
lib/connect-redis.js
};
/**
* Inherit from `Store`.
+ * Store is an EventEmitter so RedisStore is one too
@tj
Owner
tj added a line comment Sep 3, 2012

this is weird :p

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@louischatriot

I renamed the 'reconnect' event to 'connect' to match the version in Connect, and removed the useless comment.

@tj tj merged commit 07f98c3 into tj:master Sep 5, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment