DEV: Allow validity of lock to be customizable for DistributedMutex. (#7025)

- Allows a user to override the default lock validity of 60 seconds.
- Also clean up test which was leaking a redis key

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

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