Fixed the socket adapter config when using ioredis #25

jeremy-albuixech · 2016-05-19T18:24:14Z

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

Let me know if you have any questions !

sgress454 · 2016-05-19T19:14:55Z

Hi @Albi34

Couple of things here:

Can you take a look at why the tests are failing?
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 · 2016-05-19T21:37:05Z

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 · 2016-05-19T23:33:57Z

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'
},

…pecified client config.

Salakar · 2016-06-27T14:38:45Z

@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 · 2016-06-27T14:59:23Z

@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 · 2016-06-27T15:02:02Z

@sgress454 Ok, thanks for keeping us posted.

mikermcneil · 2016-06-27T17:08:56Z

@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 · 2016-08-05T16:02:33Z

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

mikermcneil

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!

mikermcneil · 2016-11-03T01:47:18Z

lib/configure.js

@@ -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) {


Can we name this redisClientOpts instead?

mikermcneil · 2016-11-03T01:49:52Z

lib/configure.js

+    if (app.config.sockets.adapterClientConfig) {
+      app.config.sockets.adapterOptions.adapterClientConfig = app.config.sockets.adapterClientConfig;
+    }
+    if (app.config.sockets.adapterClientName) {


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

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

mikermcneil · 2016-11-03T01:50:32Z

lib/prepare-driver.js

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


(See above about renaming this redisClientOpts)

mikermcneil · 2016-11-03T01:51:06Z

lib/prepare-driver.js

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


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

var redisLibrary = adapterConfig.redisLibrary;

mikermcneil · 2016-11-03T01:52:21Z

lib/prepare-driver.js


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


=> adapterConfig.redisClientOpts (see above)

mikermcneil · 2016-11-03T02:11:40Z

test/with-other-redis-client.test.js

+
+    loadHooks: ['moduleloader', 'userconfig', 'http', 'sockets'],
+
+    // Configure the socket.io-redis adapter


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?

mikermcneil · 2016-11-03T02:13:05Z

package.json

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


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

mikermcneil · 2016-11-03T02:15:00Z

lib/prepare-driver.js

      // If `pass` was supplied, pass it in as `auth_pass`
      if (adapterConfig.pass) {
-
-        redisClientOpts.auth_pass = adapterConfig.pass;
+        adapterClientConfig.auth_pass = adapterConfig.pass;


=> redisClientOpts

mikermcneil · 2016-11-03T02:15:35Z

lib/prepare-driver.js

        }
      }

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


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

mikermcneil · 2016-11-03T02:20:47Z

lib/prepare-driver.js

-
+      var initiateRedisClient = function(adapterConfig, redis, adapterClientName, adapterClientConfig){
+        if(adapterClientName === 'ioredis'){
+          return new redis(adapterClientConfig);


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.

^^ 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).

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

mikermcneil · 2016-11-03T02:47:59Z

@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 · 2016-11-03T03:49:44Z

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

cc @sgress454

jeremy-albuixech · 2016-11-03T04:23:09Z

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 · 2016-11-16T18:25:01Z

@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 · 2016-12-07T03:52:53Z

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

istrau2 · 2017-12-05T17:24:28Z

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

Changed config naming convention and ensure creating raw clients if s…

9f46132

…pecified client config.

jeremy-albuixech added 2 commits August 5, 2016 11:00

Merge with upstream master

ab67cf1

Added a case for another redis client in the tests

5ec33f3

jeremy-albuixech added 2 commits August 5, 2016 12:07

Moved packages to dev deps

5f691cd

password property for ioredis instead of pass

1a6d239

mikermcneil requested changes Nov 3, 2016

View reviewed changes

mikermcneil mentioned this pull request Dec 7, 2016

Session and Socket cannot use adapter 'ioredis' balderdashy/sails#3840

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed the socket adapter config when using ioredis #25

Fixed the socket adapter config when using ioredis #25

jeremy-albuixech commented May 19, 2016

sgress454 commented May 19, 2016 •

edited

jeremy-albuixech commented May 19, 2016 •

edited

jeremy-albuixech commented May 19, 2016

Salakar commented Jun 27, 2016

sgress454 commented Jun 27, 2016

jeremy-albuixech commented Jun 27, 2016

mikermcneil commented Jun 27, 2016

jeremy-albuixech commented Aug 5, 2016

mikermcneil left a comment

mikermcneil Nov 3, 2016

mikermcneil Nov 3, 2016

mikermcneil Nov 3, 2016

mikermcneil Nov 3, 2016

mikermcneil Nov 3, 2016

mikermcneil Nov 3, 2016

mikermcneil Nov 3, 2016

mikermcneil Nov 3, 2016

mikermcneil Nov 3, 2016

mikermcneil Nov 3, 2016

mikermcneil Nov 3, 2016

mikermcneil Nov 3, 2016

mikermcneil Nov 3, 2016

mikermcneil commented Nov 3, 2016

mikermcneil commented Nov 3, 2016

jeremy-albuixech commented Nov 3, 2016

mikermcneil commented Nov 16, 2016

mikermcneil commented Dec 7, 2016 •

edited

istrau2 commented Dec 5, 2017 •

edited


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

		// Configure the socket.io-redis adapter

Fixed the socket adapter config when using ioredis #25

Are you sure you want to change the base?

Fixed the socket adapter config when using ioredis #25

Conversation

jeremy-albuixech commented May 19, 2016

sgress454 commented May 19, 2016 • edited

jeremy-albuixech commented May 19, 2016 • edited

jeremy-albuixech commented May 19, 2016

Salakar commented Jun 27, 2016

sgress454 commented Jun 27, 2016

jeremy-albuixech commented Jun 27, 2016

mikermcneil commented Jun 27, 2016

jeremy-albuixech commented Aug 5, 2016

mikermcneil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikermcneil commented Nov 3, 2016

mikermcneil commented Nov 3, 2016

jeremy-albuixech commented Nov 3, 2016

mikermcneil commented Nov 16, 2016

mikermcneil commented Dec 7, 2016 • edited

istrau2 commented Dec 5, 2017 • edited

sgress454 commented May 19, 2016 •

edited

jeremy-albuixech commented May 19, 2016 •

edited

mikermcneil commented Dec 7, 2016 •

edited

istrau2 commented Dec 5, 2017 •

edited