Use SET NX/EX for new master locking approach

CHANGELOG.md

@@ -5,6 +5,7 @@
 ## [Unreleased]
 ### Changed
 - Remove support for Ruby < 2.3
+- Add `Lock::ResilientModern` that uses GET with NX/EX instead of Lua script to set master lock ([#734](https://github.com/resque/resque-scheduler/pull/734))
 
 ## [4.5.0] - 2021-09-25
 ### Added

lib/resque/scheduler/lock.rb

@@ -1,4 +1,4 @@
 # vim:fileencoding=utf-8
-%w(base basic resilient).each do |file|
+%w(base basic resilient resilient_modern).each do |file|
   require "resque/scheduler/lock/#{file}"
 end

lib/resque/scheduler/lock/resilient_modern.rb

@@ -0,0 +1,14 @@
+# vim:fileencoding=utf-8
+require_relative 'base'
+
+module Resque
+  module Scheduler
+    module Lock
+      class ResilientModern < Resilient
+        def acquire!
+          Resque.redis.set(key, value, nx: true, ex: timeout)
+        end
+      end
+    end
+  end
+end

lib/resque/scheduler/locking.rb

@@ -50,6 +50,7 @@
 # were missed when it starts up again.
 
 require_relative 'lock'
+require 'rubygems/version'
 
 module Resque
   module Scheduler
@@ -59,7 +60,11 @@ def master_lock
       end
 
       def supports_lua?
-        redis_master_version >= 2.5
+        redis_master_version >= Gem::Version.new('2.5')
+      end
+
+      def supports_get_x_options?
+        redis_master_version >= Gem::Version.new('2.6.12')
       end
 
       def master?
@@ -83,7 +88,9 @@ def release_master_lock
       private
 
       def build_master_lock
-        if supports_lua?
+        if supports_get_x_options?
+          Resque::Scheduler::Lock::ResilientModern.new(master_lock_key)
+        elsif supports_lua?
           Resque::Scheduler::Lock::Resilient.new(master_lock_key)
         else
           Resque::Scheduler::Lock::Basic.new(master_lock_key)
@@ -97,7 +104,7 @@ def master_lock_key
       end
 
       def redis_master_version
-        Resque.data_store.redis.info['redis_version'].to_f
+        Gem::Version.new(Resque.data_store.redis.info['redis_version'])
       end
     end
   end

test/scheduler_locking_test.rb

@@ -7,6 +7,77 @@ def lock_is_not_held(lock)
   end
 end
 
+module LockSharedTests
+  # rubocop:disable Metrics/MethodLength
+  # rubocop:disable Metrics/AbcSize
+  def self.included(mod)
+    mod.class_eval do
+      test 'you should not have the lock if someone else holds it' do
+        lock_is_not_held(@lock)
+
+        assert !@lock.locked?
+      end
+
+      test 'you should not be able to acquire the lock if someone ' \
+          'else holds it' do
+        lock_is_not_held(@lock)
+
+        assert !@lock.acquire!
+      end
+
+      test 'the lock should receive a TTL on acquiring' do
+        @lock.acquire!
+
+        assert Resque.redis.ttl(@lock.key) > 0, 'lock should expire'
+      end
+
+      test 'releasing should release the master lock' do
+        assert @lock.acquire!, 'should have acquired the master lock'
+        assert @lock.locked?, 'should be locked'
+
+        @lock.release!
+
+        assert !@lock.locked?, 'should not be locked'
+      end
+
+      test 'checking the lock should increase the TTL if we hold it' do
+        @lock.acquire!
+        Resque.redis.setex(@lock.key, 10, @lock.value)
+
+        @lock.locked?
+
+        assert Resque.redis.ttl(@lock.key) > 10, 'TTL should have been updated'
+      end
+
+      test 'checking the lock should not increase the TTL if we do not hold it' do
+        Resque.redis.setex(@lock.key, 10, @lock.value)
+        lock_is_not_held(@lock)
+
+        @lock.locked?
+
+        assert Resque.redis.ttl(@lock.key) <= 10,
+               'TTL should not have been updated'
+      end
+
+      test 'setting the lock timeout changes the key TTL if we hold it' do
+        @lock.acquire!
+
+        @lock.stubs(:locked?).returns(true)
+        @lock.timeout = 120
+        ttl = Resque.redis.ttl(@lock.key)
+        assert_send [ttl, :>, 100]
+
+        @lock.stubs(:locked?).returns(true)
+        @lock.timeout = 180
+        ttl = Resque.redis.ttl(@lock.key)
+        assert_send [ttl, :>, 120]
+      end
+    end
+  end
+  # rubocop:enable Metrics/AbcSize
+  # rubocop:enable Metrics/MethodLength
+end
+
 context '#master_lock_key' do
   setup do
     @subject = Class.new { extend Resque::Scheduler::Locking }
@@ -94,14 +165,22 @@ def lock_is_not_held(lock)
     assert_equal @subject.master_lock.class, Resque::Scheduler::Lock::Basic
   end
 
-  test 'should use the resilient lock mechanism for > Redis 2.4' do
-    Resque.redis.stubs(:info).returns('redis_version' => '2.5.12')
+  test 'should use the resilient lock mechanism for > Redis 2.4 and < 2.6.12' do
+    Resque.data_store.redis.stubs(:info).returns('redis_version' => '2.5.12')
 
     assert_equal(
       @subject.master_lock.class, Resque::Scheduler::Lock::Resilient
     )
   end
 
+  test 'should use the resilient lock mechanism for >= Redis 2.6.12' do
+    Resque.data_store.redis.stubs(:info).returns('redis_version' => '2.8.0')
+
+    assert_equal(
+      @subject.master_lock.class, Resque::Scheduler::Lock::ResilientModern
+    )
+  end
+
   test 'should be the master if the lock is held' do
     @subject.master_lock.acquire!
     assert @subject.master?, 'should be master'
@@ -153,52 +232,7 @@ def lock_is_not_held(lock)
     @lock.release!
   end
 
-  test 'you should not have the lock if someone else holds it' do
-    lock_is_not_held(@lock)
-
-    assert !@lock.locked?
-  end
-
-  test 'you should not be able to acquire the lock if someone ' \
-       'else holds it' do
-    lock_is_not_held(@lock)
-
-    assert !@lock.acquire!
-  end
-
-  test 'the lock should receive a TTL on acquiring' do
-    @lock.acquire!
-
-    assert Resque.redis.ttl(@lock.key) > 0, 'lock should expire'
-  end
-
-  test 'releasing should release the master lock' do
-    assert @lock.acquire!, 'should have acquired the master lock'
-    assert @lock.locked?, 'should be locked'
-
-    @lock.release!
-
-    assert !@lock.locked?, 'should not be locked'
-  end
-
-  test 'checking the lock should increase the TTL if we hold it' do
-    @lock.acquire!
-    Resque.redis.setex(@lock.key, 10, @lock.value)
-
-    @lock.locked?
-
-    assert Resque.redis.ttl(@lock.key) > 10, 'TTL should have been updated'
-  end
-
-  test 'checking the lock should not increase the TTL if we do not hold it' do
-    Resque.redis.setex(@lock.key, 10, @lock.value)
-    lock_is_not_held(@lock)
-
-    @lock.locked?
-
-    assert Resque.redis.ttl(@lock.key) <= 10,
-           'TTL should not have been updated'
-  end
+  include LockSharedTests
 end
 
 context 'Resque::Scheduler::Lock::Resilient' do
@@ -216,11 +250,7 @@ def lock_is_not_held(lock)
       @lock.release!
     end
 
-    test 'you should not have the lock if someone else holds it' do
-      lock_is_not_held(@lock)
-
-      assert !@lock.locked?, 'you should not have the lock'
-    end
+    include LockSharedTests
 
     test 'refreshes sha cache when the sha cannot be found on ' \
          'the redis server' do
@@ -233,62 +263,6 @@ def lock_is_not_held(lock)
       assert_false @lock.acquire!
     end
 
-    test 'you should not be able to acquire the lock if someone ' \
-         'else holds it' do
-      lock_is_not_held(@lock)
-
-      assert !@lock.acquire!
-    end
-
-    test 'the lock should receive a TTL on acquiring' do
-      @lock.acquire!
-
-      assert Resque.redis.ttl(@lock.key) > 0, 'lock should expire'
-    end
-
-    test 'releasing should release the master lock' do
-      assert @lock.acquire!, 'should have acquired the master lock'
-      assert @lock.locked?, 'should be locked'
-
-      @lock.release!
-
-      assert !@lock.locked?, 'should not be locked'
-    end
-
-    test 'checking the lock should increase the TTL if we hold it' do
-      @lock.acquire!
-      Resque.redis.setex(@lock.key, 10, @lock.value)
-
-      @lock.locked?
-
-      assert Resque.redis.ttl(@lock.key) > 10, 'TTL should have been updated'
-    end
-
-    test 'checking the lock should not increase the TTL if we do ' \
-         'not hold it' do
-      Resque.redis.setex(@lock.key, 10, @lock.value)
-      lock_is_not_held(@lock)
-
-      @lock.locked?
-
-      assert Resque.redis.ttl(@lock.key) <= 10,
-             'TTL should not have been updated'
-    end
-
-    test 'setting the lock timeout changes the key TTL if we hold it' do
-      @lock.acquire!
-
-      @lock.stubs(:locked?).returns(true)
-      @lock.timeout = 120
-      ttl = Resque.redis.ttl(@lock.key)
-      assert_send [ttl, :>, 100]
-
-      @lock.stubs(:locked?).returns(true)
-      @lock.timeout = 180
-      ttl = Resque.redis.ttl(@lock.key)
-      assert_send [ttl, :>, 120]
-    end
-
     test 'setting lock timeout is a noop if not held' do
       @lock.acquire!
       @lock.timeout = 100
@@ -325,3 +299,22 @@ def lock_is_not_held(lock)
     end
   end
 end
+
+context 'Resque::Scheduler::Lock::ResilientModern' do
+  include LockTestHelper
+
+  if !Resque::Scheduler.supports_get_x_options?
+    puts '*** Skipping Resque::Scheduler::Lock::ResilientModern ' \
+         'tests, as they require Redis >= 2.6.2.'
+  else
+    setup do
+      @lock = Resque::Scheduler::Lock::ResilientModern.new('test_resilient_modern_lock')
+    end
+
+    teardown do
+      @lock.release!
+    end
+
+    include LockSharedTests
+  end
+end