extend_life interprets TTL as seconds rather than milliseconds (0.1.8, fixed on master)

[thbar]

I'm rather new with redlock-rb so bear with me 😄

While doing extensive tests I think I stumbled upon a bug on version 0.1.8 (I'm using redis 3.0.7 and ruby 2.2.3 on that specific test).

I believe when extend_life: true is passed, 0.1.8 interprets the TTL value as seconds, rather than milliseconds. Master (commit 7d9c91a) will interpret the value as milliseconds (as I'd have expected).

I'm running this:

redis.flushall
ap "==================="
lock_info = lock_manager.lock('test', 2000)
5.times do
  ap redis.pttl('test')
  sleep(0.1)
end
ap "==================="
lock_info = lock_manager.lock('test', 3000, extend: lock_info, extend_life: true)
5.times do
  ap redis.pttl('test')
  sleep(0.1)
end

Output on 0.1.8

"==================="
1999
1898
1797
1696
1594
"==================="
3000000
2999898
2999797
2999696
2999594

Output on master

"==================="
1999
1899
1797
1695
1594
"==================="
2999
2897
2783
2681
2579

[thbar]

Suggestion : we could have tests relying on some range of the pttl value to avoid regressions on that!

[thbar]

One last note: while doing those tests I also noticed that extend life will actually reset the TTL (but not increase it). This could also be tested and added to the documentation as well (since it wasn't clear at all if the extension would add to the existing TTL, rather than replace it). Just a thought!

[maltoe]

@thbar You're right. Good catch. In 0.1.8 the script still uses the Redis EXPIRE command which takes a millisecond parameter, and on master we made lock + extend use the same script with Redis SET command and PX parameter to expire keys, which takes seconds.

@leandromoreira What do you think? The version on master could break clients "relying" on this behaviour of extend in the released version of the gem. Although I doubt this feature has too many users (yet), so I would vote for changing it anyways and putting a warning in the README.

@thbar Would you be willing to write those specs? Also you're right that the README could be more explicit about what extend does to the TTL.

[leandromoreira]

Thanks @thbar great catch!

I would vote for changing it anyways and putting a warning in the README.

Yes 👍

[thbar]

@maltoe I'll be writing those specs, and maybe ask a few questions in the PR (ex: extend_life vs. extend_only_if_life difference, etc!).

[thbar]

@maltoe here is the PR - #45. I'll work on a README update later in the week-end. Feel free to comment on the PR itself!

[maltoe]

This is all fixed and we have TTL specs