redlock-rb icon indicating copy to clipboard operation
redlock-rb copied to clipboard

> 2.0.0 .lock! no longer works

Open hirowatari opened this issue 1 year ago • 3 comments

Minimal reproduction

lock_manager = Redlock::Client.new(["redis://redis:6379/1"])
retry_count = 50
retry_delay = 100 # 100 milliseconds
lock_time = 5_000 # 5 seconds
(0..9).map do |i|
  Thread.new do
    lock_manager.lock!('lockname', lock_time, retry_count: retry_count, retry_delay: retry_delay) do
      puts "Thread #{i} got lock"
      sleep 0.2
      puts "Thread #{i} released lock"
    end
  end
end.each(&:join)

Output in version 1.3.2

Thread 3 got lock
Thread 3 released lock
Thread 8 got lock
Thread 8 released lock
Thread 7 got lock
Thread 7 released lock
Thread 5 got lock
Thread 5 released lock
Thread 6 got lock
Thread 6 released lock
Thread 9 got lock
Thread 9 released lock
Thread 4 got lock
Thread 4 released lock
Thread 2 got lock
Thread 2 released lock
Thread 0 got lock
Thread 0 released lock
Thread 1 got lock
Thread 1 released lock

This is consistent and I have a test that looks similar to this (with some real work instead of sleeping) that has been working in production and relying on this behaviour for a long time.

Output in version 2.0.2


Thread 4 got lock
Thread 4 released lock
Thread 8 got lock
Thread 8 released lock
Thread 0 got lock
Thread 0 released lock
Thread 2 got lock
Thread 9 got lock
Thread 3 got lock
Thread 1 got lock
Thread 7 got lock
Thread 2 released lock
Thread 9 released lock
Thread 3 released lock
Thread 6 got lock
Thread 1 released lock
Thread 5 got lock
Thread 7 released lock
Thread 6 released lock
Thread 5 released lock

Have I misunderstood how this is supposed to work, or is this a bug? I had expected only one thread to be able to get the lock at the same time.

hirowatari avatar Jul 13 '23 19:07 hirowatari

From this comment I see that perhaps I should change

lock_manager = Redlock::Client.new(["redis://redis:6379/1"])
<snip>
(0..9).map do |i|
  Thread.new do
    lock_manager.lock!('lockname', lock_time, retry_count: retry_count, retry_delay: retry_delay) do
<snip>

to

<snip>
(0..9).map do |i|
  Thread.new do
    Redlock::Client.new(["redis://redis:6379/1"]).lock!('lockname', lock_time, retry_count: retry_count, retry_delay: retry_delay) do
<snip>

Is that the correct fix?

I'm sorry but I don't quite understand why my usage is wrong but would appreciate any advice on this.

hirowatari avatar Jul 13 '23 19:07 hirowatari

@hirowatari This appears to be fixed in the latest version of the gem. Try the same thing on 2.0.4.

jhuckabee avatar Aug 31 '23 12:08 jhuckabee

I was chasing this same problem and it was definitely the fact that a RedisClient instance is not thread-safe.

taylorthurlow avatar Dec 29 '23 02:12 taylorthurlow

I upgraded to 2.0.6 today and all is well. Thank you.

hirowatari avatar Apr 15 '24 16:04 hirowatari