Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

can we allow no setting of the ttl in redis and let is use allkeys-lru for its eviction policy? #66

Closed
kocodude opened this Issue Nov 13, 2012 · 15 comments

Comments

Projects
None yet
3 participants

No description provided.

Collaborator

wavded commented Aug 18, 2014

closing this case as older, please reopen if you want to discuss more

@wavded wavded closed this Aug 18, 2014

@wavded I'd like to discuss this more. There are some use cases where you wouldn't want someone to be logged out after some fixed amount of time. If you use connect-redis for an api that is being used by an app. You usually don't want that someone has to log in again after not using your app for a while.

Collaborator

wavded commented Oct 24, 2014

@despairblue this should be able to be handled in the express-session cookie options, we pull that value to set the TTY and if a cookie is going to expire then, doesn't seem to make sense to keep the session object around.

https://github.com/expressjs/session#cookiemaxage

Setting maxAge does put the redis TTL and the cookie's expiration date in sync:

127.0.0.1:6379> get sess:8RSdkTNnCARsocU0KuUVC9wUvAAytVnW
"{\"cookie\":{\"originalMaxAge\":60000,\"expires\":\"2014-10-24T17:27:41.279Z\",\"secure\":true,\"httpOnly\":true,\"path\":\"/\"},\"passport\":{}}"
127.0.0.1:6379> ttl sess:8RSdkTNnCARsocU0KuUVC9wUvAAytVnW
(integer) 55

But by default the cookies from express-session don't have maxAge nor an expiration date set:

127.0.0.1:6379> get sess:gUlG3DAtQyb6Rk5gLVElG23tHPFBAFA_
"{\"cookie\":{\"originalMaxAge\":null,\"expires\":null,\"secure\":true,\"httpOnly\":true,\"path\":\"/\"},\"passport\":{}}"
127.0.0.1:6379> ttl sess:gUlG3DAtQyb6Rk5gLVElG23tHPFBAFA_
(integer) 86381

This leads to the cookie being still valid on the client side but already removed from redis. Maybe this has been fixed in later versions, but I'm still using 1.5.
So if it has been fixed in 2, let me know, I'll try to back port the fix myself.

Collaborator

wavded commented Oct 24, 2014

Right, but a null expiry is just a browser session which expires on browser close so we don't really know how long it's going to be open (in that case its defaulted to one day as a "sensible" default (which is arguable)). If no session exists in redis, express-session should setup a new one. Or am I missing something?

Well, it will generate a new one, yes, but the examples above are from a user that hasn't logged in. If he logs in, the passport property will contain the ID to the user object in our database. So if the user does not use the app for a day he will be logged out and has to log in again. Which is ok for a website, but not for a mobile app (that's what we use it for).

Collaborator

wavded commented Oct 24, 2014

@despairblue hmm... i'm not comfortable completely eliminating a TTL, that has been the default behavior for a long time, there isn't any acceptable workarounds (e.g. req.session.regenerate or setting a far future cookie expiry)?

How about not changing the default behavior? :)

Let's say, setting ttl = -1 would just not set the ttl instead of throwing a redis exception?

Collaborator

wavded commented Oct 24, 2014

I'm game with that. So if ttl is set to -1 then we'll use a SET command instead. Care to start a PR?

Will do, but don't expect it before Monday :)

@wavded wavded reopened this Oct 25, 2014

Collaborator

wavded commented Oct 25, 2014

On second thought, I kinda loath 'magic' values, let's just add a new boolean option disableTTL so it's clearer. Thanks for working on that.

Ok. What should be the behavior if both are supplied?

Collaborator

wavded commented Oct 26, 2014

disableTTL trumps if they specify a ttl

Collaborator

wavded commented Oct 28, 2014

Fixed in 1a2f708

@wavded wavded closed this Oct 28, 2014

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