diff --git a/CHANGELOG.md b/CHANGELOG.md index dd9d705e..e6d8d738 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/resque/scheduler/lock.rb b/lib/resque/scheduler/lock.rb index ca7cbe48..fe04200d 100644 --- a/lib/resque/scheduler/lock.rb +++ b/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 diff --git a/lib/resque/scheduler/lock/resilient_modern.rb b/lib/resque/scheduler/lock/resilient_modern.rb new file mode 100644 index 00000000..d792f5eb --- /dev/null +++ b/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 diff --git a/lib/resque/scheduler/locking.rb b/lib/resque/scheduler/locking.rb index a541edcc..964a0794 100644 --- a/lib/resque/scheduler/locking.rb +++ b/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 diff --git a/test/scheduler_locking_test.rb b/test/scheduler_locking_test.rb index e665180e..261b1685 100644 --- a/test/scheduler_locking_test.rb +++ b/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