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

Fixed the socket adapter config when using ioredis #25

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

Conversation

jeremy-albuixech
Copy link
Contributor

This is a fix for the issue balderdashy/sails-hook-sockets/issues/24

Let me know if you have any questions !

@sgress454
Copy link
Member

sgress454 commented May 19, 2016

Hi @Albi34

Couple of things here:

  1. Can you take a look at why the tests are failing?
  2. Can you split the subEvent config option into a separate PR? This one is potentially more a patch, if it fixes issues that the _.cloneDeep is causing. Adding a new config option is something you'd make a proposal for (see the Sails contribution guidelines). Sorry if this seems like overkill for such a small thing, but the guidelines help keep us sane while maintaining a growing number of repos!

Thanks!

@jeremy-albuixech
Copy link
Contributor Author

jeremy-albuixech commented May 19, 2016

Hey @sgress454, thanks for the comment !

I'm a bit in an impasse, figuring out why the tests failed made me realize why the _.cloneDeep was used in the first place.

From what I gathered, the sails sockets hook creates 2 sets of pub/sub, one for the app itself and one for the admin bus. The solution I proposed will not work since it will reuse the same pub and sub clients for the two "channels" (instead of having two distinct adapters we have only one that we reuse).

What I would propose is to instantiate the ioredis clients directly within the prepare-driver.js script.

There is already something kind of similar using the node-redis module:

  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
      }));

My solution would be to allow to configure which client to use here, basically offering users the choice between the basic node-redis or the more advanced ioredis.

For that purpose I would implement the following changes :

  • Add a new option for the sockets configuration, named "adapterClientConfig". This would allow setting all the configuration for the adapter, since ioredis might have properties that node-redis does not I think it would be better that way.
  • Add a new option for the redis client to use. Right now it's by default using node-redis, I would include an option named "adapterClient" which would take the value of the Redis client to use ("ioredis" or "node-redis"). This would be then used to require the correct client in the prepare-driver script and to use the correct constructor (createClient for node-redis and new for Ioredis).
  • Make the necessary changes in the prepare-driver to instantiate the correct client depending on the two new configuration properties.
  • Here is what the config would look like:

      sockets: {
        adapter: 'socket.io-redis',
        adapterClientConfig: { 
          db: 1,
          sentinels: [{ host: 'localhost', port: 5000 }, { host: 'localhost', port: 5001 }, { host: 'localhost', port: 5002 }],
          name: 'mymaster',      
          subEvent: "messageBuffer"
        },
        adapterClient: 'ioredis'
      },
    

    That way, we'd keep the _.cloneDeep function and this PR will be mainly about these new configuration options, offering to use a different redis client to use within the socket.io-redis adapter.

    Not sure if that fits with the way you want this module to work though, any thoughts ? I'll update the PR with this once I'm done but I would understand if you find this not generic enough.

@jeremy-albuixech
Copy link
Contributor Author

I updated the PR with the solution I described above.

It's working for my specific use-case.

It should not create any conflict with the current configuration, I made sure that all unit tests are passing.

If you want to merge it, let me know and I'll include some additional unit tests to check the new features.

It's used in conjunction with the other PR here: /pull/26

For reference, here is the configuration I'm using in my sails app:

var sentinelConfig = {
  db: 1,
  sentinels: [{ host: 'localhost', port: 5000 }, { host: 'localhost', port: 5001 }, { host: 'localhost', port: 5002 }],
  name: 'mymaster'
};

[...]

sockets: {
    adapter: 'socket.io-redis',
    adapterClientConfig: sentinelConfig,
    subEvent: "messageBuffer",
    adapterClientName: 'ioredis'
}, 

@Salakar
Copy link
Contributor

Salakar commented Jun 27, 2016

@sgress454 @mikermcneil Is this going to be merged and released at some point? At the moment relying on @Albi34 's git repo on npm installs, which isn't a great solution.

Anyone with a proper failover production redis setup would be be using ioredis, node_redis does does not have sentinel or cluster support yet (we're working on this, but we're currently in the process of standardising both redis libs to use common packages etc)

@sgress454
Copy link
Member

@Salakar @Albi34 Sorry for the delay, and thanks for the work on this. It's skating towards the edge of, "this should be its own hook", since that was the point of federating these core functional units into their own modules in the first place, but we will discuss and post back soon.

@jeremy-albuixech
Copy link
Contributor Author

@sgress454 Ok, thanks for keeping us posted.

@mikermcneil
Copy link
Member

@Albi34 @sgress454 @Salakar Hey guys, as long as this maintains full compatibility, the necessary tweaks are made to make the tests pass, and we update the sails.config.sockets documentation, I'm ok with this being in sails-hook-sockets. Thanks for the help!

@jeremy-albuixech
Copy link
Contributor Author

Sorry for the delay, I just merged my changes with the latest master here and made a duplicate of the basic unit tests suite for the "with-redis" test case but with another client (ioredis, which I added in the packages.json + the latest version of socket-io.redis which supports this client).

Copy link
Member

@mikermcneil mikermcneil left a comment

Choose a reason for hiding this comment

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

I went through and took a look at everything. My takeaway is that this is a solid approach, but it's not something we can maintain for Sails (because e.g. why just ioredis? what about >>insert redis client library here<< etc.)

So instead, I'd like to tackle this a little differently: by exposing a createClient function (or comparable) in the config.

I need to get a bit more up to speed about what has changed here before I can comment further, but just wanted to take the time to give you feedback ASAP.

Thanks for all your hard work, and for getting us on the right track!

@@ -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?

if (app.config.sockets.adapterClientConfig) {
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

@@ -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';
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)

@@ -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;


// 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)


loadHooks: ['moduleloader', 'userconfig', 'http', 'sockets'],

// Configure the socket.io-redis adapter
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to extrapolate these tests in some way to prevent duplication, but I can help with that later. But just want to check first-- is this identical to the existing tests other than the change to the redis client library in the config?

@@ -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?

// 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

}
}

// Set up object that will be used for redis client options
var redisClientOpts = {};
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


var initiateRedisClient = function(adapterConfig, redis, adapterClientName, adapterClientConfig){
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

@mikermcneil
Copy link
Member

@Albi34 hmm... would you say it's true that, before this PR, you could achieve the same effect by doing the following in your config?

pubClient: (function (){
  var ioredis = require('ioredis');
  return new ioredis({
    // ... my custom io redis opts
  });
})(),

subClient: (function (){
  var ioredis = require('ioredis');
  return new ioredis({
    // ... my custom io redis opts
  });
})(),

@mikermcneil
Copy link
Member

@Albi34 I'm under the impression that e7a534e#diff-a1f6827ffd3f94fc77c76b6c445eaf27R37

cc @sgress454

@jeremy-albuixech
Copy link
Contributor Author

Hi @mikermcneil, thanks for all the input !

I'll try to go through it tomorrow when I have some time, it's been pretty busy lately :)

I have to admit that I haven't checked the code base in the past couple of months (since the PR was opened back in May) so you're right, it's possible that the changes I tried to implement here are now supported in the current master.

For your question about the changes being supported at the time I created this PR, it was in fact not possible to do it through the config because of the following lines in the prepare-adapter.js:

#37 var adapterConfig = _.cloneDeep(app.config.sockets.adapterOptions);
#66 var adminAdapterConfig = _.cloneDeep(app.config.sockets.adapterOptions);

The lodash cloneDeep function was not able to properly clone the ioredis client (I investigated it at the time, I remember it had to do with the way the ioredis client was initialized but I can't recall all the details).

Since these two lines are still present, I guess it's still not supporting Ioredis, except if the client changed in the meantime or if lodash had an update concerning the cloneDeep function, I'll give it a try as soon as I can.

@mikermcneil
Copy link
Member

@Albi34 ahh ok gotcha. So it sounds like maybe the solution here is just to remove the cloning? We should still trace the code to make sure that doesn't break any assumptions, but off the top of my head, that shouldn't be a problem. If you do have a moment to do a PR with that (or change this one), that'd be awesome, otherwise Scott or I will follow up as soon as we can. Thanks again!

@mikermcneil
Copy link
Member

mikermcneil commented Dec 7, 2016

@Albi34 just confirmed with @sgress454 and there are now tests that use ioredis

@istrau2
Copy link

istrau2 commented Dec 5, 2017

@sgress454 @mikermcneil I am trying to upgrade to sails^1.0.0 specifically so that I could use socket.io^2.0.0 and ioredis. What should I do in order to do that?

It seems the current version of sails-hook-sockets on npm uses a v1.7.3 of socket.io (which is incompatible with the latest version of socket.io-redis)

nvm, I pulled master and got it working with socket.io^2.0.0 Is there any reason that the latest version of master isn't on npm?

I'd put this comment in an issue but it doesn't look like issues are enabled for this repo...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants