diff --git a/lib/distributed_mutex.rb b/lib/distributed_mutex.rb index d47f059d0fd..ea3ca4830de 100644 --- a/lib/distributed_mutex.rb +++ b/lib/distributed_mutex.rb @@ -1,8 +1,9 @@ # Cross-process locking using Redis. class DistributedMutex + DEFAULT_VALIDITY = 60 - def self.synchronize(key, redis = nil, &blk) - self.new(key, redis).synchronize(&blk) + def self.synchronize(key, redis: nil, validity: DEFAULT_VALIDITY, &blk) + self.new(key, redis).synchronize(validity: DEFAULT_VALIDITY, &blk) end def initialize(key, redis = nil) @@ -15,16 +16,16 @@ class DistributedMutex CHECK_READONLY_ATTEMPT ||= 10 # NOTE wrapped in mutex to maintain its semantics - def synchronize - + def synchronize(validity: DEFAULT_VALIDITY) @mutex.lock attempts = 0 - while !try_to_get_lock + while !try_to_get_lock(validity) sleep 0.001 # in readonly we will never be able to get a lock if @using_global_redis && Discourse.recently_readonly? attempts += 1 + if attempts > CHECK_READONLY_ATTEMPT raise Discourse::ReadOnly end @@ -40,18 +41,20 @@ class DistributedMutex private - def try_to_get_lock + def try_to_get_lock(validity) got_lock = false - if @redis.setnx @key, Time.now.to_i + 60 - @redis.expire @key, 60 + + if @redis.setnx @key, Time.now.to_i + validity + @redis.expire @key, validity got_lock = true else begin @redis.watch @key time = @redis.get @key + if time && time.to_i < Time.now.to_i got_lock = @redis.multi do - @redis.set @key, Time.now.to_i + 60 + @redis.set @key, Time.now.to_i + validity end end ensure diff --git a/spec/components/distributed_mutex_spec.rb b/spec/components/distributed_mutex_spec.rb index 3975ea0fc1e..09f0028d1d7 100644 --- a/spec/components/distributed_mutex_spec.rb +++ b/spec/components/distributed_mutex_spec.rb @@ -2,9 +2,15 @@ require 'rails_helper' require_dependency 'distributed_mutex' describe DistributedMutex do + let(:key) { "test_mutex_key" } + + after do + $redis.del(key) + end + it "allows only one mutex object to have the lock at a time" do mutexes = (1..10).map do - DistributedMutex.new("test_mutex_key") + DistributedMutex.new(key) end x = 0 @@ -22,9 +28,9 @@ describe DistributedMutex do end it "handles auto cleanup correctly" do - m = DistributedMutex.new("test_mutex_key") + m = DistributedMutex.new(key) - $redis.setnx "test_mutex_key", Time.now.to_i - 1 + $redis.setnx key, Time.now.to_i - 1 start = Time.now.to_i m.synchronize do @@ -35,8 +41,26 @@ describe DistributedMutex do expect(Time.now.to_i).to be <= start + 1 end + it 'allows the validity of the lock to be configured' do + freeze_time + + mutex = DistributedMutex.new(key) + + mutex.synchronize(validity: 2) do + expect($redis.ttl(key)).to eq(2) + expect($redis.get(key).to_i).to eq(Time.now.to_i + 2) + end + + mutex.synchronize do + expect($redis.ttl(key)).to eq(DistributedMutex::DEFAULT_VALIDITY) + + expect($redis.get(key).to_i) + .to eq(Time.now.to_i + DistributedMutex::DEFAULT_VALIDITY) + end + end + it "maintains mutex semantics" do - m = DistributedMutex.new("test_mutex_key") + m = DistributedMutex.new(key) expect { m.synchronize do @@ -55,7 +79,7 @@ describe DistributedMutex do end it "works even if redis is in readonly" do - m = DistributedMutex.new("test_readonly") + m = DistributedMutex.new(key) start = Time.now done = false