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

connect-redis throws error when custom ioredis client is passed in config/session.js #4262

Closed
utsavkansara opened this issue Dec 14, 2017 · 13 comments
Labels

Comments

@utsavkansara
Copy link

utsavkansara commented Dec 14, 2017

Sails version:1.0.0-40
Node version:8.9.0
NPM version:5.5.1
DB adapter name: connect-redis
DB adapter version: 3.2.0
Operating system:MacOS Sierra


I am trying to make redis work with sentinel. The only way possible with SailsV1 is to create and pass custom ioredis client in config.

I am doing following in my config/session.js

var Redis = require('ioredis');
var redis = new Redis({
sentinels: [{ host: 'localhost', port: 26379 }],
name: 'mymaster'
});

module.exports.session = {
secret: '73ae3b02fa3a0d4d4dd56ab28c1f09bb',
adapter: 'connect-redis',
client: redis,
ttl: 5000,
db: 0,
pass: '',
prefix: 'session:',
}

My redis instance is running at localhost:6380
My sentinel is running at: 26379
Info on sentinel : master0:name=mymaster,status=ok,address=127.0.0.1:6380,slaves=0,sentinels=1

After doing this configuration I am hitting sails-lift which gives me following error

info: Starting app...

error: A hook (session) failed to load!
error:
error: Error: Redis connection to 127.0.0.1:6379 failed - connect ECONNREFUSED 127.0.0.1:6379
at Object._errnoException (util.js:1024:11)
at _exceptionWithHostPort (util.js:1046:20)
at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1182:14)

error: Could not load Sails app.
error:
error: Tips:
error: • First, take a look at the error message above.
error: • Check that you're using the latest stable version of Sails.
error: • Have a question or need help? (http://sailsjs.com/support)

First question comes to my mind is why it is trying to hit 127.0.0.1:6379 ?

@sailsbot
Copy link

@utsavkansara Thanks for posting, we'll take a look as soon as possible.


For help with questions about Sails, click here. If you’re interested in hiring @sailsbot and her minions in Austin, click here.

@sgress454
Copy link
Member

@utsavkansara Please see #4256

@utsavkansara
Copy link
Author

@sgress454
In my particular case what do you think my url should be ?
ultimate goal is I want to connect to redis via sentinel so that if redis master goes down, sentinel will provide new master info to redis client (ioredis)

Based on your suggestion I provided url argument to my session config which points to redis sentinel.

module.exports.session = {
secret: '73ae3b02fa3a0d4d4dd56ab28c1f09bb',
adapter: 'connect-redis',
client: redis,
url: 'redis://localhost:26379',
ttl: 5000,
db: 0,
pass: '',
prefix: 'session:',
}

Question 1: Why do we need to provide url if we are already providing connection info in redis client
Question 2: What should be url value when using sentinel

@sgress454
Copy link
Member

@utsavkansara Did the above config solve your problem? I understand the confusion -- it's probably redundant for Sails to have to make a client connection to Redis at all when it's only using pub/sub and you're already providing separate connections for that, but at the moment the adapter's not quite smart enough to figure that out. So I think any url that successfully connects will be fine.

@utsavkansara
Copy link
Author

utsavkansara commented Dec 15, 2017

@sgress454 Sails is able to come up but its not able to store session in redis.
So the issue is I am providing sentinel address in the url, so sails tries to run SET command on sentinel instead of using passed ioredis client. I get following error

{ ReplyError: ERR unknown command 'set'
at parseError (/Users/ukansara/Documents/dev/Git/SailsEngine/node_modules/redis-parser/lib/parser.js:193:12)
at parseType (/Users/ukansara/Documents/dev/Git/SailsEngine/node_modules/redis-parser/lib/parser.js:303:14)
command: 'SET',
args:
[ 'session:Gj7MwBjh45_Loj1g9ltgGmEEaS1yhtRw',
'{"cookie":{"httpOnly":true,"path":"/"}}',
'EX',
5000 ],
code: 'ERR' }

The ultimate goal of using sentinel is we don't have to connect to redis instance directly. Instead, we connect to sentinel and it will provide current redis master's info and client should be able to connect to current master.

Did I make myself clear ?

@utsavkansara
Copy link
Author

Hey @sgress454,
Do you think I would be able to go past this ?

@sgress454
Copy link
Member

@utsavkansara sorry for the delay, traveling at the moment. I think we might have gotten steered wrong here -- the link I provided above referred to sockets configure, not session. It's hard for me to access at the moment but in our Gitter chat I think I dug up the correct syntax for specifying a custom Redis client for session, please take a look at that and see if it helps.

@utsavkansara
Copy link
Author

@sgress454 Thanks for responding. Yes you are right. My particular request was for session not sockets. I am passing custom client for session based on your recommendation only. I have attached configuration information in ticket also. Please check whenever you get free.

@utsavkansara
Copy link
Author

@sgress454 Awaiting your reply. :-)

@sgress454
Copy link
Member

sgress454 commented Jan 8, 2018 via email

@sgress454
Copy link
Member

@utsavkansara, ok, I think I have this figured out. In order to use ioredis, rather than setting adapter to the string connect-redis, you'll need to set it to a the required module itself. For example.

// config/session.js
var Redis = require('ioredis');
module.exports.session = {

  secret: '94a78f441c8bb0ce433b44d80ae97a82',

  adapter: require('connect-redis'),

  client: new Redis({
    sentinels: [{ host: 'localhost', port: 26379 }],
    name: 'mymaster'
  })

};

works for me. It should work with the adapter: 'connect-redis' as well, but currently there's a bug preventing that, which I'm looking into patching right now. Thanks for your patience -- please give this a try and see if it works for your use case!

@utsavkansara
Copy link
Author

utsavkansara commented Jan 9, 2018

@sgress454 works as expected as per your suggested change.
Thanks :-)

@sgress454
Copy link
Member

🎉 Closing now that v1.0 is officially released.

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

No branches or pull requests

3 participants