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.

Sign up for GitHub

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

Fixed the socket adapter config when using ioredis #25

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Fixed the socket adapter config when using ioredis #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9f46132
Changed config naming convention and ensure creating raw clients if s…
jeremy-albuixech Jun 1, 2016
ab67cf1
Merge with upstream master
jeremy-albuixech Aug 5, 2016
5ec33f3
Added a case for another redis client in the tests
jeremy-albuixech Aug 5, 2016
5f691cd
Moved packages to dev deps
jeremy-albuixech Aug 5, 2016
1a6d239
password property for ioredis instead of pass
jeremy-albuixech Aug 5, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions lib/configure.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,12 @@ module.exports = function ToConfigure(app) {
if (app.config.sockets.subClient) {
app.config.sockets.adapterOptions.subClient = app.config.sockets.subClient;
}
if (app.config.sockets.adapterClientConfig) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we name this redisClientOpts instead?

app.config.sockets.adapterOptions.adapterClientConfig = app.config.sockets.adapterClientConfig;
}
if (app.config.sockets.adapterClientName) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of expecting the package name, can we allow an override to be passed in from userland? i.e. if app.config.sockets.redisLibrary is defined, then the automatic require() below can be skipped. Also, this way, none of the existing auto-require() stuff will need to change

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also note that this means that the redis client library can be a direct dependency of your app, allowing you to switch out to use any *redis client library in the future

app.config.sockets.adapterOptions.adapterClientName = app.config.sockets.adapterClientName;
}
if (app.config.sockets.subEvent) {
app.config.sockets.adapterOptions.subEvent = app.config.sockets.subEvent;
}
Expand Down
48 changes: 31 additions & 17 deletions lib/prepare-driver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,53 +16,67 @@ module.exports = function (app){
var adapterModuleName = adapterDef.moduleName;
var pathToAdapterDependency = adapterDef.modulePath;
var adapterConfig = adapterDef.config;
var adapterClientName = adapterConfig.adapterClientName || 'redis';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above-- instead of var adapterClientName = adapterConfig.adapterClientName || 'redis', this can be simplified to:

var redisLibrary = adapterConfig.redisLibrary;

var adapterClientConfig = adapterConfig.adapterClientConfig || {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(See above about renaming this redisClientOpts)


// For redis:
// ============================================================
if (adapterModuleName === 'socket.io-redis') {

// Create raw redis clients if necessary
var rawClientsNecessary = adapterConfig.pass || adapterConfig.db || adapterDef.adminBus;
var rawClientsNecessary = adapterConfig.pass || adapterConfig.db || adapterDef.adminBus || adapterConfig.adapterClientConfig;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=> adapterConfig.redisClientOpts (see above)

if (!rawClientsNecessary) {
return cb();
}

// Borrow access to the `redis` module from socket.io-redis
// Borrow access to the adapter redis client module from socket.io-redis
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the notes above, none of the changes to this try/catch block will be necessary anymore. Should be able to leave it exactly as it was originally, except wrap the try/catch block in if/else statement, where we use the provided redis client, if available-- and otherwise try to require redis:

if (!_.isUndefined(redisClient) {
  redis = redisLibrary;
}
else {
  // try/catch block
}

var redis;
try {
redis = require(path.resolve(pathToAdapterDependency, 'node_modules/redis'));
redis = require(adapterClientName);
}
catch (e) {
try {
redis = require('redis');
redis = require(path.resolve(pathToAdapterDependency, 'node_modules/redis'));
} catch (e2) {
return cb(e2);
try {
redis = require('redis');
} catch (e3) {
return cb(e3);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will now be at the top (see other comments) -- and let's please preserve the code comment:

 // Set up object that will be used for redis client options

}
}
}

// Set up object that will be used for redis client options
var redisClientOpts = {};

// If `pass` was supplied, pass it in as `auth_pass`
if (adapterConfig.pass) {

redisClientOpts.auth_pass = adapterConfig.pass;
adapterClientConfig.auth_pass = adapterConfig.pass;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=> redisClientOpts

}


var initiateRedisClient = function(adapterConfig, redis, adapterClientName, adapterClientConfig){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know the Sails code base hasn't always been consistent about this in the past, but we actively try to avoid inline function declarations like this. In this case, it's a 2x duplication, so I would just inline the code. If this happened more than that, then I would pull it into another file and require that.

if(adapterClientName === 'ioredis'){
return new redis(adapterClientConfig);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the tricky part to deal with-- since every redis client library has its own usage, the configured clientLibrary will have to be ducktyped, or we'd have to ask for the package name as well. It seems like a better solution here might be, instead of expecting a reference to a Redis client library and/or to the package name, we accepted a createClient function.... I'll follow up on that momentarily.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^ a la what Express does with consolidate & view engines (and what we're moving toward for sails v1). This way, we could easily include configuration for ioredis and any other redis client library that comes along in the future (meanwhile, for folks who are cool with the default redis client from inside socket.io-redis, they should be able to use everything as-is).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^and in that case, we won't need to add sails.config.sockets.redisClientOpts either, since that will be completely flexible

}
else {
return redis.createClient(adapterConfig.port || 6379, adapterConfig.host || '127.0.0.1', _.extend({}, adapterClientConfig,{
return_buffers: true
}));
}
}
// Build Redis clients if necessary
if (adapterConfig.pubClient) {
app.log.verbose('adapterConfig.pubClient already specified!! (app running on port %d)', app.config.port);
adapterConfig.pubClient = adapterConfig.pubClient;
}
else {
adapterConfig.pubClient = initiateRedisClient(adapterConfig, redis, adapterClientName, adapterClientConfig);
}

if (adapterConfig.subClient) {
app.log.verbose('adapterConfig.subClient already specified!! (app running on port %d)', app.config.port);
adapterConfig.subClient = adapterConfig.subClient
}
else {
adapterConfig.subClient = initiateRedisClient(adapterConfig, redis, adapterClientName, adapterClientConfig);
}
adapterConfig.pubClient = adapterConfig.pubClient || redis.createClient(adapterConfig.port || 6379, adapterConfig.host || '127.0.0.1', _.extend({}, redisClientOpts, {
return_buffers: true
}));
adapterConfig.subClient = adapterConfig.subClient || redis.createClient(adapterConfig.port || 6379, adapterConfig.host || '127.0.0.1', _.extend({}, redisClientOpts,{
return_buffers: true
}));

// Listen for connection errors from redis clients
// (and handle the first one if necessary)
Expand Down
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@
"mocha": "^2.2.4",
"sails": "balderdashy/sails",
"sails.io.js": "^0.13.0",
"socket.io-redis": "^1.0.0"
"ioredis": "^2.2.0",
"socket.io-redis": "git+https://github.com/socketio/socket.io-redis.git"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you leave the SVR of socket.io-redis as is or change it to^1.1.1?

},
"scripts": {
"test": "node ./node_modules/mocha/bin/mocha test/** --timeout 5000"
Expand Down
Loading