Skip to content

various fixes on redis as sessions db#1064

Merged
kataras merged 3 commits intokataras:masterfrom
akiraho:master
Aug 18, 2018
Merged

various fixes on redis as sessions db#1064
kataras merged 3 commits intokataras:masterfrom
akiraho:master

Conversation

@akiraho
Copy link
Copy Markdown
Contributor

@akiraho akiraho commented Aug 16, 2018

  1. lifetime.Time not updated on expiration shift

  2. redis service.TTL() return values were incorrect and incompatible wtih its consumers such as redis database.Acquire(); this fix re-aligns the return values so:

  • hasExpiration means there's a ttl value (i.e. > -1)
  • found means the key is found, i.e. not(redis cmd error || ttl expired)
  1. redis service.getKeysConn() previously returned keys as is from scanning the db
  • inside getKeysConn() it scans redis with "r.Config.Prefix+prefix"
  • previously it returned the found keys as is
  • then UpdateTTLMany() would call updateTTLConn() with such keys
  • and updateTTLConn() would again perform redis expire with "r.Config.Prefix+key"
  • this causes the redis expire to always fail as the call to redis expire would be with duplicated prefixes
  • with this fix service.getKeysConn() expects a passed in key param without config.prefix, and also return keys without config.prefix, so things are aligned and config.prefix won't get duplicated

@akiraho akiraho requested a review from kataras as a code owner August 16, 2018 10:15
Copy link
Copy Markdown
Owner

@kataras kataras left a comment

Choose a reason for hiding this comment

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

Hello @akiraho,

That's good but the fix / lifetime.Time not updated on expiration shift is wrong, we can't just add the duration from the current time. The shift expiration does not "adds" lifetime, it is re-set its to the configured, with the current commit you pushed you just extend it not reset it: if a session entry had 20 mins to be expired, and the configured was 30minutes, then you make it 50minutes instead of 30minutes (which is what ShiftExpiration does). Time should be re-seted, not add.

@akiraho
Copy link
Copy Markdown
Contributor Author

akiraho commented Aug 18, 2018

Hi @kataras,

  • e.g. sessions.New(Config{Expires: 30m})

  • @08:00, sessions.Start()
    => session.Lifetime.Begin(30m) // will expire @08:30
    => inside Lifetime.begin(), lt.Time set to 08:30 with "time.Now().Add(30m)"

  • @08:20, sessions.ShiftExpiration() // should now expire @08:50, i.e. shift by 30m
    => provider.UpdateExpiration(30m)
    => session.Lifetime.Shift(30m); database.OnUpdateExpiration(30m)
    => inside session.Lifetime.Shift()
    current code: lt.timer.Reset(30m) // timer will expire after 30m, starting from now
    I added: lt.Time = time.Now().Add(30m) // i.e. lt.Time = 08:50

So the result is that everything points to the new expiration @08:50.

Isn't that what we want to achieve?

Thanks.

@kataras
Copy link
Copy Markdown
Owner

kataras commented Aug 18, 2018

Oh, excuse me @akiraho I seen lifetime.Time.Add, now I see that you used the time.Now(), it's perfect then, good job!!!

@kataras kataras merged commit 1efa53e into kataras:master Aug 18, 2018
github-actions Bot pushed a commit to goproxies/github.com-kataras-iris that referenced this pull request Jul 27, 2020
various fixes on redis as sessions db

Former-commit-id: 0fb804f095c37d340eeaf595806329a67c5b43ac
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants