From 29106ebc4bb1b75f1fdd8566b8d361cdf77a205b Mon Sep 17 00:00:00 2001 From: Pranav Raj S Date: Fri, 19 Jan 2024 02:31:41 -0800 Subject: [PATCH] fix: raise_lock_acquisition_error if the job cannot set the lock (#8744) Consider a scenario where two jobs are concurrently accessing a job with a lock mechanism. If the second job accesses lock_manager.locked? before the first job called lock_manager.lock(lock_key), it would return a false positive. At this point, both jobs can be executed freely. To address this issue, the following change ensures that only the current thread that has set the lock can execute. Otherwise, a LockAcquisitionError will be thrown. Co-authored-by: Sojan Jose Co-authored-by: Shivam Mishra --- app/jobs/mutex_application_job.rb | 16 +++++++--------- spec/jobs/mutex_application_job_spec.rb | 5 +---- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/app/jobs/mutex_application_job.rb b/app/jobs/mutex_application_job.rb index d6d20b1e2..98e6bf9cd 100644 --- a/app/jobs/mutex_application_job.rb +++ b/app/jobs/mutex_application_job.rb @@ -18,17 +18,15 @@ class MutexApplicationJob < ApplicationJob lock_key = format(key_format, *args) lock_manager = Redis::LockManager.new - if lock_manager.locked?(lock_key) - Rails.logger.warn "[#{self.class.name}] Failed to acquire lock on attempt #{executions}: #{lock_key}" - raise LockAcquisitionError, "Failed to acquire lock for key: #{lock_key}" - end - begin - lock_manager.lock(lock_key) - Rails.logger.info "[#{self.class.name}] Acquired lock for: #{lock_key} on attempt #{executions}" - yield + if lock_manager.lock(lock_key) + Rails.logger.info "[#{self.class.name}] Acquired lock for: #{lock_key} on attempt #{executions}" + yield + else + Rails.logger.warn "[#{self.class.name}] Failed to acquire lock on attempt #{executions}: #{lock_key}" + raise LockAcquisitionError, "Failed to acquire lock for key: #{lock_key}" + end ensure - # Ensure that the lock is released even if there's an error in processing lock_manager.unlock(lock_key) end end diff --git a/spec/jobs/mutex_application_job_spec.rb b/spec/jobs/mutex_application_job_spec.rb index 8483708f7..919195c32 100644 --- a/spec/jobs/mutex_application_job_spec.rb +++ b/spec/jobs/mutex_application_job_spec.rb @@ -16,14 +16,12 @@ RSpec.describe MutexApplicationJob do before do allow(Redis::LockManager).to receive(:new).and_return(lock_manager) - allow(lock_manager).to receive(:locked?).and_return(false) allow(lock_manager).to receive(:lock).and_return(true) allow(lock_manager).to receive(:unlock).and_return(true) end describe '#with_lock' do it 'acquires the lock and yields the block if lock is not acquired' do - expect(lock_manager).to receive(:locked?).with(lock_key).and_return(false) expect(lock_manager).to receive(:lock).with(lock_key).and_return(true) expect(lock_manager).to receive(:unlock).with(lock_key).and_return(true) @@ -31,7 +29,7 @@ RSpec.describe MutexApplicationJob do end it 'raises LockAcquisitionError if it cannot acquire the lock' do - allow(lock_manager).to receive(:locked?).with(lock_key).and_return(true) + allow(lock_manager).to receive(:lock).with(lock_key).and_return(false) expect do described_class.new.send(:with_lock, lock_key) do @@ -41,7 +39,6 @@ RSpec.describe MutexApplicationJob do end it 'ensures that the lock is released even if there is an error during block execution' do - expect(lock_manager).to receive(:locked?).with(lock_key).and_return(false) expect(lock_manager).to receive(:lock).with(lock_key).and_return(true) expect(lock_manager).to receive(:unlock).with(lock_key).and_return(true)