Fixes #73 ReplaceIfEqualAsync sets a wrong expiration value causing RenewAsync to release locks

[niemyjski]

Looks like dequeue id had the same problem. I also noticed that our ReplaceIfEqualAsync base unit tests doesn't check expiration but I didn't have time to rework that.

Docs: https://redis.io/commands/set

[syntx]

@niemyjski @ejsmith Thanks for fixing this fast, however I have a comment about the implantation (added to the code). Did you consider the simpler fix that I suggested?

[ejsmith]

@syntx I'm not seeing anything. Where are the comments?

[syntx]

@ejsmith I added my comment in the resolved discussion in ReplaceIfEqual.lua. I wrote that I think You should keep the value passed to the script in milliseconds since the Redis SET command only accepts integer values. I think that by that you are keeping things more consistent since that, for instance, for the locking scenario - the original lock expiration can be set in millisecond precision and you should allow the renewal to take place with the same precision.

[ejsmith]

@syntx I think seconds is probably a pretty granular precision. The EXPIRE command in Redis only works with seconds and the entire expiration in Redis is a best effort system that isn't exact down to the millisecond precision. Remember also that this is the lock timeout and shouldn't come into play normally as when the lock is released it's removed immediately.

[syntx]

@ejsmith I believe that Redis supports millisecond precsision expiration since version 2.6 At least according to their page at https://redis.io/commands/expire - see under "Expire accuracy".

As a user of the library, It would make more sense to me that AddAsync and ReplaceIfEqualAsync in this same class operate in a consistent manner. If one already allows setting the expiration in milliseconds precision the other should not behave differently.

Also, note that TimeSpan.TotalSeconds that you've used here returns a double, I'm not sure how the Lua script would handle cases like mine, with sub second precision, since if you try passing a fractional expiration number to Redis in a SET command you'd normally get a value is not an integer or out of range error.

[syntx]

To clarify more, I noticed that in AddAsync you're actually calling RedisDatabase.StringSetAsync from StackExchange.Redis which has a very granular handling of expirations, as shown here: https://github.com/StackExchange/StackExchange.Redis/blob/51d2946b7f1499605d90b95a00bcb8f776ff0b49/src/StackExchange.Redis/RedisDatabase.cs#L3472-L3492 The code in the above link handles both the rounding of ticks from the given TimeSpan value into milliseconds and then selects the correct version of SET command with either PX or EX to express expiration in up to millisecond accuracy.

Both of these measures are missing from the Lua implementation in ReplaceIfEqualAsync

The missing rounding, especially, can create unintended value is not an integer or out of range errors.

[ejsmith]

@syntx you are right. I’ll take another pass at this.

[niemyjski]

Just adding some comments: I did think of using milliseconds at first but I too thought the resolution of seconds was good. It appears that TotalMilliseconds returns a double as well.

[ejsmith]

@syntx can you take a look at https://github.com/FoundatioFx/Foundatio.Redis/pull/75