# A more efficient Redis lock

**URL:** <https://meta.discourse.org/t/a-more-efficient-redis-lock/177841>\
**Category:** Development\
**Created:** [January 31, 2021, 1:10pm UTC](https://meta.discourse.org/t/a-more-efficient-redis-lock/177841 "2021-01-31T13:10:02Z")\
**Posts on this page:** 2\
**Page:** 1

<div class="post-metadata">

**Author:** ![concerto](https://avatars.discourse-cdn.com/v4/letter/c/dec6dc/32.png) [@concerto](https://meta.discourse.org/u/concerto)\
**Post date:** [January 31, 2021, 1:10pm UTC](https://meta.discourse.org/t/a-more-efficient-redis-lock/177841/1 "2021-01-31T13:10:02Z")

</div>

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](https://redis.io/topics/transactions). the `lock` process could be simplified into the code below.

```ruby
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](https://redis.io/commands/setex) 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](https://github.com/discourse/discourse/commit/ed45a1dce363e734642b78e238f88d9874be7680), we did not use the TTL provided by Redis. The TTL feature is introduced [much later](https://github.com/discourse/discourse/commit/7c7098c70056e1a3c2a8a9b366a793994b62dee8).

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

---

<div class="post-metadata">

**Author:** ![sam](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/sam/32/102149_2.png) [@sam](https://meta.discourse.org/u/sam)\
**Post date:** [January 31, 2021, 11:22pm UTC](https://meta.discourse.org/t/a-more-efficient-redis-lock/177841/2 "2021-01-31T23:22:15Z")

</div>

Per [Develop with Redis | Docs](https://redis.io/topics/distlock)

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