A more efficient Redis lock

Dev
1

Discourse use Redis lock widely as a way of synchronization. Discourse’s implementation of Redis lock(as show below) can be improved to use less Redis command, reducing round-trip time and cost for Redis to execute them.

https://github.com/discourse/discourse/blob/363170513e7be3e7fa1500ac640561e0e3c45e78/lib/distributed_mutex.rb#L84-L106

How we may improve it

Both lock and unlock uses Redis Transaction. the lock process could be simplified into the code below.

WATCH key
GET key # determine whether the key has expired
MULTI
SET key
EXPIRE key, expire_time + 1

The reason to use Redis Transaction (I assumed) seems to be that it checks whether the key is expired before actually setting the key.

But I think we could just use the SETEX command provided be Redis, which sets a key with a expiration time. In fact, the Redis SETEX Documentation use it as a example to replace SET & EXPIRE with SETEX.

Here is the arguments for why we should replace it:

  1. Setting the value to expiration time and checks it before setting the key is unnecessary. As the TTL mechanism is enough to ensure that key expires correctly.
  2. Even if we decide to use expiration time as value, we do not need any transaction. Since provding atomicity between the GET (line 2 in code above) and MULTI & EXEC did not provide anything. This is because that if the lock is not acquired, it will retry.

A Historical note

I dig into the git commit history for more information. It seems like that when the Redis lock is introduced, we did not use the TTL provided by Redis. The TTL feature is introduced much later.

So I guess we could take one step further and remove Redis Transaction all the way.

4 Likes
2

Per Distributed locks with Redis – Redis

We can probably junk the whole transaction and do a set with with the px and nx options

3 Likes