Atomic increment and expire

[simonhaines]

Raised by olegkv in this thread:

As far as I see, there is still race condition there, because call to Redis INCR command and a call to set expiration are different calls and counter may change in between.

Indeed the key creation and expiry is a little loose and in very rare cases can create keys that never expire or expire too early. The former case is a (very rare) problem for long-running caches, and I've had a crack at tightening it up a little on the branch atomic-incr-expireat (PRs welcome etc).

However the expectation for rate-limiting middleware is low latency, so this is preferred over formal correctness (within limits). I'll profile the branch and see how it goes. From the failure scenarios you describe, it sounds like your use-case requires consensus in a network of failing nodes, so maybe you're better served by something like an implementation of Raft?

[olegkv]

With Lua script such as:

local current
current = redis.call("incr",KEYS[1])
if tonumber(current) == 1 then
redis.call("expire",KEYS[1],1)
end

it will be bullet proof atomic, no race conditions at all.
StackExchange.Redis supports Lua scripts, so it seems rather easy to solve all race conditions once and for all.
Also, see this:
https://redis.io/commands/incr

[simonhaines]

Looks to me like a great candidate for a pull request!

[olegkv]

Also, Lua is good because it moves time logic to Redis which means it will use central (Redis) time.
With your transaction logic here
https://github.com/statusas/AspNetCoreRateLimit/commits/atomic-incr-expireat
it is atomic but depends on local time. If you have two machines with different time and the difference is comparable to rate limit intervals, counters will not work correctly, each machine may work in different interval.

[simonhaines]

@olegkv so... no pull request then?

[simonhaines]

This commit adds an atomic 'set and expire key' operation to the rate limit counter store.