Incrementing and decrementing key does not preserve its expiration (i.e. TTL)

[mpxc8102]

Ruby Version: 2.3.5
Readthis Version: 2.0.2
Environment: macOS Sierra 10.12.6

If you open up a Redis command line, set a key/value with a TTL (time to live), then increment the key, the TTL stays:

redis> SET x 1 'EX' 99999
OK
redis> TTL x
(integer) 99997
redis> INCR x
(integer) 2
redis> TTL x
(integer) 99994

If you do the same with a Readthis client however, the TTL will disappear:

irb(main):006:0> readthis_client.write('x', '1', expires_in: 99999)
=> "OK"
irb(main):007:0> readthis_client.pool { |c| c.ttl('x') }
=> 99997
irb(main):008:0> readthis_client.increment('x')
=> 2
irb(main):009:0> readthis_client.pool { |c| c.ttl('x') }
=> -1

It seems the cause is #increment and #decrement don't call the incr and decr Redis commands, but instead have their own implementation where they read the existing value and then write over it. But while doing so, these methods don't preserve the TTL of the key.

It seems we're not calling incr and decr because Redis can't actually increment or decrement values for Readthis if Readthis encodes and marshals all values it sends to Redis. See 139553b

[mpxc8102]

I'm not sure if there are other options like expires_in that #increment and #decrement override.

[sorentwo]

@mattvleming Thanks for the report. I've spent a bit of time trying to re-work the way increment and decrement work, with the goal of using incrby and decrby directly. You are correct that the way encoding and compression work is entirely incompatible with the counter commands. We're left with a couple of options:

• Retain the existing non-atomic read/write, but update it to include the expiration
• Make increment/decrement use the corresponding Redis commands, but make it clear that they will not work with values written by Readthis.

The first option keeps the convenience of the library, but breaks atomicity and makes writing with expiration require four commands (read, read ttl, write, write ttl). The second option retains atomicity, simplifies the commands, but requires raw use of the redis client to initialize a value.

I'm a bit torn on the direction right now. Input from others is welcome.

[mpxc8102]

@sorentwo Ya it's a tricky one. I played around with the Readthis::Passthrough marshal class, hoping that with this Marshal Readthis would set the values into Redis as plaintext. However, when I used that Marshal class, the values stored in Redis by Readthis were still encoded. I think it might have been this line doing that.

I wonder if in addition to the first option you suggested above, you could provide developers a second option. So they two together would be:

1. Allow developers to enable marshalling and encoding, but explain to them this will impact the performance of increment and decrement (because, as you explained above, you will have to run four commands)
2. Or allow developers to disable marshalling and encoding, and be able to take advantage of the native incr and decr Redis commands

Obviously this adds complexity to configuration of the gem, which is not great.

That's one solution (allowing developers to turn on or off marshalling and encoding, which would influence the execution of increment and decrement). But I've just thought of another solution. Check this out:

redis> SET 'x' 'y'
OK
redis> INCR 'x'
(error) ERR value is not an integer or out of range

A developer should never call increment and decrement on a key that isn't an integer. Also, you don't need to marshal values that are integers. So what you could do is, if the value that comes through #write is an integer, then skip marshalling and encoding it, and save the value as a plain integer into Redis. That way you can use the native incr and decr Redis commands on the key of that value.

And then if the developer tries to increment and decrement a key that doesn't have an integer value, you can catch that error from Redis and raise an exception to the developer saying you can't increment or decrement a value that isn't an integer.

This means that #read will have to be modified to not de-marshal and decode the value from Redis if the value is an integer.