diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md index b73537336..d6d0bf549 100644 --- a/.github/ISSUE_TEMPLATE/bug_report.md +++ b/.github/ISSUE_TEMPLATE/bug_report.md @@ -7,29 +7,25 @@ about: Create a report to help us improve **Describe the bug** A clear and concise description of what the bug is. -**To Reproduce** -Steps to reproduce the behavior: -1. Go to '...' -2. Click on '....' -3. Scroll down to '....' -4. See error - **Expected behavior** A clear and concise description of what you expected to happen. -**Screenshots** -If applicable, add screenshots to help explain your problem. +**Current behavior** +What happens instead of the expected behavior? + +**Worker class** -**Desktop (please complete the following information):** - - OS: [e.g. iOS] - - Browser [e.g. chrome, safari] - - Version [e.g. 22] +```ruby +class MyWorker + include Sidekiq::Worker + sidekiq_options unique: :until_executed, queue: :undefault + def perform(args); end -**Smartphone (please complete the following information):** - - Device: [e.g. iPhone6] - - OS: [e.g. iOS8.1] - - Browser [e.g. stock browser, safari] - - Version [e.g. 22] + def self.unique_args(args) + # the way you consider unique arguments + end +end +``` **Additional context** Add any other context about the problem here. diff --git a/.gitignore b/.gitignore index b3584b370..4785f39cd 100644 --- a/.gitignore +++ b/.gitignore @@ -20,3 +20,7 @@ rails_example/spec/examples.txt .rspec_status /gemfiles/.bundle/ + +/.byebug_history + +/.yardoc/ diff --git a/README.md b/README.md index 7ff394619..c3a788265 100644 --- a/README.md +++ b/README.md @@ -2,27 +2,20 @@ The missing unique jobs for sidekiq -# Documentation +## Documentation -This is the documentation for the master branch. You can find the documentation for each release by navigating to its tag: https://github.com/mhenrixon/sidekiq-unique-jobs/tree/v5.0.10. +This is the documentation for the master branch. You can find the documentation for each release by navigating to its tag: https://github.com/mhenrixon/sidekiq-unique-jobs/tree/v5.0.10. Below are links to the latest major versions (4 & 5): + - [v5.0.10](https://github.com/mhenrixon/sidekiq-unique-jobs/tree/v5.0.10) - [v4.0.18](https://github.com/mhenrixon/sidekiq-unique-jobs/tree/v4.0.18) ## Requirements -See https://github.com/mperham/sidekiq#requirements for what is required. Starting from 5.0.0 only sidekiq >= 4 is supported and support for MRI <= 2.1 is dropped. ActiveJob is not supported - -Version 5 requires redis >= 3 - -### ActiveJob +See https://github.com/mperham/sidekiq#requirements for what is required. Starting from 5.0.0 only sidekiq >= 4 is supported and support for MRI <= 2.1 is dropped. ActiveJob is not supported -Due to the simplicity of ActiveJob and the complexity of this game there is no officially supported way of doing ActiveJob. If you want to use uniqueness you should be using sidekiq directly. I know some projects started by using ActiveJob out of ignorance and someone has to do a whole lot of work to migrate the workers to use sidekiq directly... - -If you are in this position and you can't figure it out; I have done such migrations for really big clients before. I am a consultant with a ton of experience on such jobs. My rate is fair and I am easy to get along with. - -If that is not an option I apologize. This gem won't support ActiveJob moving forward. It would require monkey patching too much. +Version 6 requires Redis >= 3 and pure Sidekiq, no ActiveJob supported anymore. See [About ActiveJob](https://github.com/mhenrixon/sidekiq-unique-jobs/wiki/About-ActiveJob) for why. ## Installation @@ -38,15 +31,17 @@ Or install it yourself as: $ gem install sidekiq-unique-jobs -## Locking +## General Information + +See [Interaction w/ Sidekiq](https://github.com/mhenrixon/sidekiq-unique-jobs/wiki/How-this-gem-interacts-with-Sidekiq) on how the gem interacts with Sidekiq. -Sidekiq consists of a client and a server. The client is responsible for pushing jobs to the queue and the server is responsible for actually processing the jobs. When the client puts the job to the queue the middleware checks for uniqueness and creates a lock. When the server then processes the job that lock is released. +See [Locking & Unlocking](https://github.com/mhenrixon/sidekiq-unique-jobs/wiki/Locking-&-Unlocking) for an overview of the differences on when the various lock types are locked and unlocked. ### Options #### Lock Expiration -This is probably not the configuration option you want... +This is probably not the configuration option you want... Since the client and the server are disconnected and not running inside the same process, setting a lock expiration is probably not what you want. Any keys that are used by this gem WILL be removed at the time of the expiration. For jobs that are scheduled in the future the key will expire when that job is scheduled + whatever expiration you have set. @@ -67,7 +62,54 @@ sidekiq_options lock_timeout: 5 # wait 5 seconds sidekiq_options lock_timeout: nil # lock indefinitely, this process won't continue until it gets a lock. VERY DANGEROUS!! ``` -#### +#### Unique Across Queues + +This configuration option is slightly misleading. It doesn't disregard the queue on other jobs. Just on itself, this means that a worker that might schedule jobs into multiple queues will be able to have uniqueness enforced on all queues it is pushed to. + +```ruby +class Worker + include Sidekiq::Worker + + sidekiq_options: unique_across_queues: true, queue: 'default' + + def perform(args); end +end +``` + +Now if you push override the queue with `Worker.set(queue: 'another').perform_async(1)` it will still be considered unique when compared to `Worker.perform_async(1)` (that was actually pushed to the queue `default`). + +#### Unique Across Workers + +This configuration option is slightly misleading. It doesn't disregard the worker class on other jobs. Just on itself, this means that a worker that the worker class won't be used for generating the unique digest. The only way this option really makes sense is when you want to have uniqueness between two different worker classes. + +```ruby +class WorkerOne + include Sidekiq::Worker + + sidekiq_options: unique_across_workers: true, queue: 'default' + + def perform(args); end +end + +class WorkerTwo + include Sidekiq::Worker + + sidekiq_options: unique_across_workers: true, queue: 'default' + + def perform(args); end +end + + +WorkerOne.perform_async(1) +# => 'the jobs unique id' + +WorkerTwo.perform_async(1) +# => nil because WorkerOne just stole the lock +``` + +### Locks + +#### ### Until Executing @@ -83,14 +125,13 @@ sidekiq_options unique: :until_executing Locks from when the client pushes the job to the queue. Will be unlocked when the server has successfully processed the job. - ```ruby sidekiq_options unique: :until_executed ``` ### Until Timeout -Locks from when the client pushes the job to the queue. Will be unlocked when the specified timeout has been reached. +Locks from when the client pushes the job to the queue. Will be unlocked when the specified timeout has been reached. ```ruby sidekiq_options unique: :until_expired @@ -106,11 +147,9 @@ sidekiq_options unique: :until_and_while_executing ### While Executing -With this lock type it is possible to put any number of these jobs on the queue, but as the server pops the job from the queue it will create a lock and then wait until other locks are done processing. It *looks* like multiple jobs are running at the same time but in fact the second job will only be waiting for the first job to finish. +With this lock type it is possible to put any number of these jobs on the queue, but as the server pops the job from the queue it will create a lock and then wait until other locks are done processing. It _looks_ like multiple jobs are running at the same time but in fact the second job will only be waiting for the first job to finish. -#### NOTE: - -Unless this job is configured with a `lock_timeout: nil` or `lock_timeout: > 0` then all jobs that are attempted to be executed will just be dropped without waiting. +**NOTE** Unless this job is configured with a `lock_timeout: nil` or `lock_timeout: > 0` then all jobs that are attempted to be executed will just be dropped without waiting. ```ruby sidekiq_options unique: :while_executing, lock_timeout: nil @@ -135,17 +174,9 @@ In the console you should see something like: 10:33:04 worker.1 | 2017-04-23T08:33:04.973Z 84404 TID-ougq8cs8s WhileExecutingWorker JID-9e197460c067b22eb1b5d07f INFO: done: 40.014 sec ``` - -### Uniqueness Scope - -- Queue specific locks -- Across all queues - [examples/unique_on_all_queues_job.rb](https://github.com/mhenrixon/sidekiq-unique-jobs/blob/master/examples/unique_on_all_queues_job.rb) -- Across all workers - [examples/unique_across_workers_job.rb](https://github.com/mhenrixon/sidekiq-unique-jobs/blob/master/examples/unique_across_workers_job.rb) -- Timed / Scheduled jobs - ## Usage -All that is required is that you specifically set the sidekiq option for *unique* to a valid value like below: +All that is required is that you specifically set the sidekiq option for _unique_ to a valid value like below: ```ruby sidekiq_options unique: :while_executing @@ -165,7 +196,7 @@ The method or the proc can return a modified version of args without the transie class UniqueJobWithFilterMethod include Sidekiq::Worker sidekiq_options unique: :until_and_while_executing, - unique_args: :unique_args + unique_args: :unique_args # this is default and will be used if such a method is defined def self.unique_args(args) [ args[0], args[2][:type] ] @@ -185,9 +216,6 @@ class UniqueJobWithFilterProc end ``` -The previous problems with unique args being string in server and symbol in client is no longer a problem because the `UniqueArgs` class accounts for this and converts everything to json now. If you find an edge case please provide and example so that we can add coverage and fix it. - - It is also quite possible to ensure different types of unique args based on context. I can't vouch for the below example but see [#203](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/203) for the discussion. ```ruby @@ -207,10 +235,12 @@ class UniqueJobWithFilterMethod end ``` - ### After Unlock Callback -If you are using :after_yield as your unlock ordering, Unique Job offers a callback to perform some work after the block is yielded. +If you need to perform any additional work after the lock has been released you can provide an `#after_unlock` instance method. The method will be called when the lock has been unlocked. Most times this means after yield but there are two exceptions to that. + +**Exception 1:** UntilExecuting unlocks and calls back before yielding. +**Exception 2:** UntilExpired expires eventually, no after_unlock hook is called. ```ruby class UniqueJobWithFilterMethod @@ -222,12 +252,11 @@ class UniqueJobWithFilterMethod end ... end. - ``` ### Logging -To see logging in sidekiq when duplicate payload has been filtered out you can enable on a per worker basis using the sidekiq options. The default value is false +To see logging in sidekiq when duplicate payload has been filtered out you can enable on a per worker basis using the sidekiq options. The default value is false ```ruby class UniqueJobWithFilterMethod @@ -241,15 +270,19 @@ end ``` ## Debugging + There are two ways to display and remove keys regarding uniqueness. The console way and the command line way. ### Console + Start the console with the following command `bundle exec jobs console`. #### List Unique Keys + `keys '*', 100` #### Remove Unique Keys + `del '*', 100, false` the dry_run and count parameters are both required. This is to have some type of protection against clearing out all uniqueness. ### Command Line diff --git a/lib/sidekiq_unique_jobs/constants.rb b/lib/sidekiq_unique_jobs/constants.rb index ea616f805..b312f3000 100644 --- a/lib/sidekiq_unique_jobs/constants.rb +++ b/lib/sidekiq_unique_jobs/constants.rb @@ -9,10 +9,11 @@ module SidekiqUniqueJobs LOCK_TIMEOUT_KEY ||= 'lock_timeout' LOG_DUPLICATE_KEY ||= 'log_duplicate_payload' QUEUE_KEY ||= 'queue' + UNIQUE_ACROSS_QUEUES_KEY ||= 'unique_across_queues' UNIQUE_ACROSS_WORKERS_KEY ||= 'unique_across_workers' UNIQUE_ARGS_KEY ||= 'unique_args' UNIQUE_DIGEST_KEY ||= 'unique_digest' UNIQUE_KEY ||= 'unique' - UNIQUE_ON_ALL_QUEUES_KEY ||= 'unique_on_all_queues' + UNIQUE_ON_ALL_QUEUES_KEY ||= 'unique_on_all_queues' # TODO: Remove in v6.1 UNIQUE_PREFIX_KEY ||= 'unique_prefix' end diff --git a/lib/sidekiq_unique_jobs/lock/base_lock.rb b/lib/sidekiq_unique_jobs/lock/base_lock.rb index 012a58b45..af27b3399 100644 --- a/lib/sidekiq_unique_jobs/lock/base_lock.rb +++ b/lib/sidekiq_unique_jobs/lock/base_lock.rb @@ -5,16 +5,18 @@ class Lock class BaseLock include SidekiqUniqueJobs::Logging - def initialize(item, redis_pool = nil) + def initialize(item, callback, redis_pool = nil) @item = prepare_item(item) + @callback = callback @redis_pool = redis_pool + @operative = true end def lock locksmith.lock(item[LOCK_TIMEOUT_KEY]) end - def execute(_callback = nil) + def execute raise NotImplementedError, "##{__method__} needs to be implemented in #{self.class}" end @@ -36,20 +38,19 @@ def locked? private - attr_reader :item, :redis_pool, :operative + attr_reader :item, :redis_pool, :operative, :callback def locksmith @locksmith ||= SidekiqUniqueJobs::Locksmith.new(item, redis_pool) end - def using_protection(callback) - @operative = true + def with_cleanup yield rescue Sidekiq::Shutdown @operative = false raise ensure - unlock_and_callback(callback) + unlock_with_callback end def prepare_item(item) @@ -60,22 +61,23 @@ def prepare_item(item) item end - def unlock_and_callback(callback) - return notify_about_manual_unlock unless operative - unlock - - return notify_about_manual_unlock if locked? - callback_safely(callback) - end - def notify_about_manual_unlock log_fatal("the unique_key: #{item[UNIQUE_DIGEST_KEY]} needs to be unlocked manually") + false + end + + def unlock_with_callback + return notify_about_manual_unlock unless operative + return notify_about_manual_unlock unless unlock + + callback_safely + item[JID_KEY] end - def callback_safely(callback) - callback.call + def callback_safely + callback&.call rescue StandardError - log_warn("the callback for unique_key: #{item[UNIQUE_DIGEST_KEY]} failed!") + log_warn("The lock for #{item[UNIQUE_DIGEST_KEY]} has been released but the #after_unlock callback failed!") raise end end diff --git a/lib/sidekiq_unique_jobs/lock/until_and_while_executing.rb b/lib/sidekiq_unique_jobs/lock/until_and_while_executing.rb index 0eebd5911..8c1388646 100644 --- a/lib/sidekiq_unique_jobs/lock/until_and_while_executing.rb +++ b/lib/sidekiq_unique_jobs/lock/until_and_while_executing.rb @@ -3,17 +3,17 @@ module SidekiqUniqueJobs class Lock class UntilAndWhileExecuting < BaseLock - def execute(callback) + def execute return unless locked? unlock - runtime_lock.execute(callback) do + runtime_lock.execute do yield if block_given? end end def runtime_lock - @runtime_lock ||= SidekiqUniqueJobs::Lock::WhileExecuting.new(item, redis_pool) + @runtime_lock ||= SidekiqUniqueJobs::Lock::WhileExecuting.new(item, callback, redis_pool) end end end diff --git a/lib/sidekiq_unique_jobs/lock/until_executed.rb b/lib/sidekiq_unique_jobs/lock/until_executed.rb index be00b0932..878f032a3 100644 --- a/lib/sidekiq_unique_jobs/lock/until_executed.rb +++ b/lib/sidekiq_unique_jobs/lock/until_executed.rb @@ -5,12 +5,9 @@ class Lock class UntilExecuted < BaseLock OK ||= 'OK' - def execute(callback) + def execute return unless locked? - using_protection(callback) do - yield if block_given? - end - unlock + with_cleanup { yield if block_given? } end end end diff --git a/lib/sidekiq_unique_jobs/lock/until_executing.rb b/lib/sidekiq_unique_jobs/lock/until_executing.rb index 1dd5ebf85..9aec613b1 100644 --- a/lib/sidekiq_unique_jobs/lock/until_executing.rb +++ b/lib/sidekiq_unique_jobs/lock/until_executing.rb @@ -3,9 +3,8 @@ module SidekiqUniqueJobs class Lock class UntilExecuting < BaseLock - def execute(callback) - unlock - callback.call + def execute + unlock_with_callback yield if block_given? end end diff --git a/lib/sidekiq_unique_jobs/lock/until_expired.rb b/lib/sidekiq_unique_jobs/lock/until_expired.rb index 5924c599e..26fee2a78 100644 --- a/lib/sidekiq_unique_jobs/lock/until_expired.rb +++ b/lib/sidekiq_unique_jobs/lock/until_expired.rb @@ -7,10 +7,10 @@ def unlock true end - def execute(callback) + def execute return unless locked? yield if block_given? - callback.call + # this lock does not handle after_unlock since we don't know when that would end end end diff --git a/lib/sidekiq_unique_jobs/lock/while_executing.rb b/lib/sidekiq_unique_jobs/lock/while_executing.rb index 4a470e03f..0e18267d8 100644 --- a/lib/sidekiq_unique_jobs/lock/while_executing.rb +++ b/lib/sidekiq_unique_jobs/lock/while_executing.rb @@ -5,8 +5,8 @@ class Lock class WhileExecuting < BaseLock RUN_SUFFIX ||= ':RUN' - def initialize(item, redis_pool = nil) - super(item, redis_pool) + def initialize(item, callback, redis_pool = nil) + super(item, callback, redis_pool) append_unique_key_suffix end @@ -17,12 +17,9 @@ def lock end # Locks the job with the RUN_SUFFIX appended - def execute(callback) - locksmith.lock(item[LOCK_TIMEOUT_KEY]) do - using_protection(callback) do - yield if block_given? - end - end + def execute + return unless locksmith.lock(item[LOCK_TIMEOUT_KEY]) + with_cleanup { yield if block_given? } end private diff --git a/lib/sidekiq_unique_jobs/lock/while_executing_reject.rb b/lib/sidekiq_unique_jobs/lock/while_executing_reject.rb index 784c4198a..4cf273809 100644 --- a/lib/sidekiq_unique_jobs/lock/while_executing_reject.rb +++ b/lib/sidekiq_unique_jobs/lock/while_executing_reject.rb @@ -3,14 +3,10 @@ module SidekiqUniqueJobs class Lock class WhileExecutingReject < WhileExecuting - def execute(callback) + def execute return reject unless locksmith.lock(item[LOCK_TIMEOUT_KEY]) - using_protection(callback) do - yield if block_given? - end - - unlock + with_cleanup { yield if block_given? } end # Private below here, keeping public due to testing reasons diff --git a/lib/sidekiq_unique_jobs/lock/while_executing_requeue.rb b/lib/sidekiq_unique_jobs/lock/while_executing_requeue.rb index e35332a2c..93a7162bc 100644 --- a/lib/sidekiq_unique_jobs/lock/while_executing_requeue.rb +++ b/lib/sidekiq_unique_jobs/lock/while_executing_requeue.rb @@ -7,12 +7,13 @@ def lock true end - def execute(callback) + def execute locksmith.lock(item[LOCK_TIMEOUT_KEY], raise: true) do - yield - callback.call + yield if block_given? end + unlock + Sidekiq::Client.push(item) unless locksmith.locked? end end diff --git a/lib/sidekiq_unique_jobs/options_with_fallback.rb b/lib/sidekiq_unique_jobs/options_with_fallback.rb index b764f1388..f74058513 100644 --- a/lib/sidekiq_unique_jobs/options_with_fallback.rb +++ b/lib/sidekiq_unique_jobs/options_with_fallback.rb @@ -34,7 +34,7 @@ def log_duplicate_payload? end def lock - @lock ||= lock_class.new(item, @redis_pool) + @lock ||= lock_class.new(item, after_unlock_hook, @redis_pool) end def lock_class diff --git a/lib/sidekiq_unique_jobs/server/middleware.rb b/lib/sidekiq_unique_jobs/server/middleware.rb index 90ee15594..8b397fe95 100644 --- a/lib/sidekiq_unique_jobs/server/middleware.rb +++ b/lib/sidekiq_unique_jobs/server/middleware.rb @@ -11,7 +11,7 @@ def call(worker_class, item, queue) @queue = queue return yield if unique_disabled? - lock.execute(after_unlock_hook) do + lock.execute do yield end end @@ -19,10 +19,6 @@ def call(worker_class, item, queue) protected attr_reader :item - - def after_unlock_hook - -> { worker_class.after_unlock if worker_method_defined?(:after_unlock) } - end end end end diff --git a/lib/sidekiq_unique_jobs/sidekiq_worker_methods.rb b/lib/sidekiq_unique_jobs/sidekiq_worker_methods.rb index 429a5f8c2..be0cd795a 100644 --- a/lib/sidekiq_unique_jobs/sidekiq_worker_methods.rb +++ b/lib/sidekiq_unique_jobs/sidekiq_worker_methods.rb @@ -19,6 +19,10 @@ def worker_class @_worker_class ||= worker_class_constantize # rubocop:disable Naming/MemoizedInstanceVariableName end + def after_unlock_hook + -> { worker_class.after_unlock if worker_method_defined?(:after_unlock) } + end + # Attempt to constantize a string worker_class argument, always # failing back to the original argument when the constant can't be found # diff --git a/lib/sidekiq_unique_jobs/unique_args.rb b/lib/sidekiq_unique_jobs/unique_args.rb index 061c91174..32712e0e8 100644 --- a/lib/sidekiq_unique_jobs/unique_args.rb +++ b/lib/sidekiq_unique_jobs/unique_args.rb @@ -54,7 +54,8 @@ def unique_args(args) end def unique_on_all_queues? - item[UNIQUE_ON_ALL_QUEUES_KEY] || worker_options[UNIQUE_ON_ALL_QUEUES_KEY] + item[UNIQUE_ACROSS_QUEUES_KEY] || worker_options[UNIQUE_ACROSS_QUEUES_KEY] || + item[UNIQUE_ON_ALL_QUEUES_KEY] || worker_options[UNIQUE_ON_ALL_QUEUES_KEY] # TODO: Remove in v 6.1 end def unique_across_workers? diff --git a/spec/integration/sidekiq_unique_jobs/lock/until_and_while_executing_spec.rb b/spec/integration/sidekiq_unique_jobs/lock/until_and_while_executing_spec.rb index ed363c518..57eb5f029 100644 --- a/spec/integration/sidekiq_unique_jobs/lock/until_and_while_executing_spec.rb +++ b/spec/integration/sidekiq_unique_jobs/lock/until_and_while_executing_spec.rb @@ -5,11 +5,11 @@ RSpec.describe SidekiqUniqueJobs::Lock::UntilAndWhileExecuting, redis: :redis, redis_db: 3 do include SidekiqHelpers - let(:process_one) { described_class.new(item_one) } - let(:runtime_one) { SidekiqUniqueJobs::Lock::WhileExecuting.new(item_one.dup) } + let(:process_one) { described_class.new(item_one, callback) } + let(:runtime_one) { SidekiqUniqueJobs::Lock::WhileExecuting.new(item_one.dup, callback) } - let(:process_two) { described_class.new(item_two) } - let(:runtime_two) { SidekiqUniqueJobs::Lock::WhileExecuting.new(item_two.dup) } + let(:process_two) { described_class.new(item_two, callback) } + let(:runtime_two) { SidekiqUniqueJobs::Lock::WhileExecuting.new(item_two.dup, callback) } let(:jid_one) { 'jid one' } let(:jid_two) { 'jid two' } @@ -48,7 +48,7 @@ it 'process two cannot lock the job' do expect(process_two.lock).to eq(nil) - expect(process_two.execute(callback)).to eq(nil) + expect(process_two.execute).to eq(nil) expect(process_two.locked?).to eq(false) end @@ -58,7 +58,7 @@ context 'when process_one executes the job in 0 seconds' do context 'when process_one executes the job' do # rubocop:disable RSpec/NestedGroups it 'process two can lock the job' do - process_one.execute(callback) do + process_one.execute do expect(process_one.locked?).to eq(false) expect(runtime_one.locked?).to eq(true) expect(process_two.lock).to eq(jid_two) @@ -67,10 +67,10 @@ end it 'process two cannot execute the job' do - process_one.execute(callback) do + process_one.execute do unset = true expect(process_two.lock).to eq(jid_two) - process_two.execute(callback) do + process_two.execute do unset = false end @@ -85,7 +85,7 @@ let(:sleepy_time) { 1 } it 'process two can lock the job' do - process_one.execute(callback) do + process_one.execute do expect(process_one.locked?).to eq(false) expect(runtime_one.locked?).to eq(true) expect(process_two.lock).to eq(jid_two) @@ -94,10 +94,10 @@ end it 'process two cannot execute the job' do - process_one.execute(callback) do + process_one.execute do unset = true expect(process_two.lock).to eq(jid_two) - process_two.execute(callback) do + process_two.execute do unset = false end @@ -113,7 +113,7 @@ context 'when process_one executes the job' do it 'process two can lock the job' do - process_one.execute(callback) do + process_one.execute do expect(process_one.locked?).to eq(false) expect(runtime_one.locked?).to eq(true) expect(process_two.lock).to eq(jid_two) @@ -122,10 +122,10 @@ end it 'process two cannot execute the job' do - process_one.execute(callback) do + process_one.execute do unset = true expect(process_two.lock).to eq(jid_two) - process_two.execute(callback) do + process_two.execute do unset = false end @@ -140,7 +140,7 @@ let(:sleepy_time) { 1 } it 'process two can lock the job' do - process_one.execute(callback) do + process_one.execute do expect(process_one.locked?).to eq(false) expect(runtime_one.locked?).to eq(true) expect(process_two.lock).to eq(jid_two) @@ -149,10 +149,10 @@ end it 'process two cannot execute the job' do - process_one.execute(callback) do + process_one.execute do unset = true expect(process_two.lock).to eq(jid_two) - process_two.execute(callback) do + process_two.execute do unset = false end diff --git a/spec/integration/sidekiq_unique_jobs/lock/until_executed_spec.rb b/spec/integration/sidekiq_unique_jobs/lock/until_executed_spec.rb index 3a9a6b542..56fbee447 100644 --- a/spec/integration/sidekiq_unique_jobs/lock/until_executed_spec.rb +++ b/spec/integration/sidekiq_unique_jobs/lock/until_executed_spec.rb @@ -5,8 +5,8 @@ RSpec.describe SidekiqUniqueJobs::Lock::UntilExecuted, redis: :redis do include SidekiqHelpers - let(:process_one) { described_class.new(item_one) } - let(:process_two) { described_class.new(item_two) } + let(:process_one) { described_class.new(item_one, callback) } + let(:process_two) { described_class.new(item_two, callback) } let(:jid_one) { 'jid one' } let(:jid_two) { 'jid two' } @@ -39,19 +39,19 @@ it 'process two cannot lock the job' do expect(process_two.lock).to eq(nil) - expect(process_two.execute(callback)).to eq(nil) + expect(process_two.execute).to eq(nil) expect(process_two.locked?).to eq(false) end context 'when process_one executes the job' do it 'the first client process should be unlocked' do - process_one.execute(callback) do + process_one.execute do expect(process_one.locked?).to eq(true) expect(process_two.lock).to eq(nil) expect(process_two.locked?).to eq(false) unset = true - process_two.execute(callback) do + process_two.execute do unset = false end diff --git a/spec/integration/sidekiq_unique_jobs/lock/until_expired_spec.rb b/spec/integration/sidekiq_unique_jobs/lock/until_expired_spec.rb index 14b9a8b85..2a08a36d0 100644 --- a/spec/integration/sidekiq_unique_jobs/lock/until_expired_spec.rb +++ b/spec/integration/sidekiq_unique_jobs/lock/until_expired_spec.rb @@ -5,8 +5,8 @@ RSpec.describe SidekiqUniqueJobs::Lock::UntilExpired, redis: :redis do include SidekiqHelpers - let(:process_one) { described_class.new(item_one) } - let(:process_two) { described_class.new(item_two) } + let(:process_one) { described_class.new(item_one, callback) } + let(:process_two) { described_class.new(item_two, callback) } let(:jid_one) { 'jid one' } let(:jid_two) { 'jid two' } @@ -51,7 +51,7 @@ it 'process two cannot execute the lock' do unset = true - process_two.execute(callback) do + process_two.execute do unset = false end @@ -60,7 +60,7 @@ it 'process one can execute the job' do set = false - process_one.execute(callback) do + process_one.execute do set = true end @@ -68,26 +68,10 @@ end it 'the job is still locked after executing' do - process_one.execute(callback) {} + process_one.execute {} expect(process_one.locked?).to eq(true) end - - it 'calls back' do - process_one.execute(callback) do - # NO OP - end - - expect(callback).to have_received(:call) - end - - it 'callbacks are only called once (for the locked process)' do - process_one.execute(callback) do - process_two.execute(callback) {} - end - - expect(callback).to have_received(:call).once - end end end diff --git a/spec/integration/sidekiq_unique_jobs/lock/while_executing_reject_spec.rb b/spec/integration/sidekiq_unique_jobs/lock/while_executing_reject_spec.rb index a13490c6b..9956dba1b 100644 --- a/spec/integration/sidekiq_unique_jobs/lock/while_executing_reject_spec.rb +++ b/spec/integration/sidekiq_unique_jobs/lock/while_executing_reject_spec.rb @@ -5,8 +5,8 @@ RSpec.describe SidekiqUniqueJobs::Lock::WhileExecutingReject, redis: :redis do include SidekiqHelpers - let(:process_one) { described_class.new(item_one) } - let(:process_two) { described_class.new(item_two) } + let(:process_one) { described_class.new(item_one, callback) } + let(:process_two) { described_class.new(item_two, callback) } let(:jid_one) { 'jid one' } let(:jid_two) { 'jid two' } @@ -41,16 +41,16 @@ context 'when job is executing' do it 'locks the process' do - process_one.execute(callback) do + process_one.execute do expect(process_one.locked?).to eq(true) end end shared_examples 'rejects job to deadset' do it 'moves subsequent jobs to dead queue' do - process_one.execute(callback) do + process_one.execute do expect(dead_count).to eq(0) - expect { process_two.execute(callback) {} } + expect { process_two.execute {} } .to change { dead_count }.from(0).to(1) end end diff --git a/spec/integration/sidekiq_unique_jobs/lock/while_executing_spec.rb b/spec/integration/sidekiq_unique_jobs/lock/while_executing_spec.rb index 432ece911..ad2b6e440 100644 --- a/spec/integration/sidekiq_unique_jobs/lock/while_executing_spec.rb +++ b/spec/integration/sidekiq_unique_jobs/lock/while_executing_spec.rb @@ -5,8 +5,8 @@ RSpec.describe SidekiqUniqueJobs::Lock::WhileExecuting, redis: :redis do include SidekiqHelpers - let(:process_one) { described_class.new(item_one) } - let(:process_two) { described_class.new(item_two) } + let(:process_one) { described_class.new(item_one, callback) } + let(:process_two) { described_class.new(item_two, callback) } let(:jid_one) { 'jid one' } let(:jid_two) { 'jid two' } @@ -45,24 +45,24 @@ context 'when job is executing' do it 'locks the process' do - process_one.execute(callback) do + process_one.execute do expect(process_one.locked?).to eq(true) end end it 'calls back' do - process_one.execute(callback) do + process_one.execute do # NO OP end expect(callback).to have_received(:call) end it 'prevents other processes from executing' do - process_one.execute(callback) do + process_one.execute do expect(process_two.lock).to eq(true) expect(process_two.locked?).to eq(false) unset = true - process_two.execute(callback) do + process_two.execute do unset = false end expect(unset).to eq(true) diff --git a/spec/support/shared_examples/an_executing_lock_with_error_handling.rb b/spec/support/shared_examples/an_executing_lock_with_error_handling.rb index ef581d0fd..252bd4204 100644 --- a/spec/support/shared_examples/an_executing_lock_with_error_handling.rb +++ b/spec/support/shared_examples/an_executing_lock_with_error_handling.rb @@ -1,9 +1,8 @@ # frozen_string_literal: true RSpec.shared_examples 'an executing lock with error handling' do - subject(:execute) { lock.execute(empty_callback, &block) } + subject(:execute) { lock.execute(&block) } - let(:empty_callback) { -> {} } let(:block) { -> {} } let(:error_message) { "the unique_key: #{item['unique_digest']} needs to be unlocked manually" } let(:initially_locked?) { true } @@ -13,29 +12,12 @@ allow(lock).to receive(:locked?).and_return(initially_locked?, locked?) allow(lock).to receive(:unlock).and_return(true) allow(lock).to receive(:delete).and_return(true) - allow(empty_callback).to receive(:call).and_call_original + allow(callback).to receive(:call).and_call_original allow(block).to receive(:call).and_call_original allow(lock).to receive(:log_warn) allow(lock).to receive(:log_fatal) end - context 'when yield fails with Sidekiq::Shutdown' do - let(:block) { -> { fail Sidekiq::Shutdown, 'testing' } } - - it 'logs a helpful error message' do - expect { execute }.to raise_error(Sidekiq::Shutdown) - - expect(lock).to have_received(:log_fatal).with(error_message) - end - - it 'raises Sidekiq::Shutdown' do - expect { execute }.to raise_error(Sidekiq::Shutdown, 'testing') - - expect(lock).not_to have_received(:unlock) - expect(empty_callback).not_to have_received(:call) - end - end - context 'when yield fails with other errors' do let(:block) { -> { raise 'HELL' } } let(:locked?) { nil } @@ -45,15 +27,5 @@ expect(lock).to have_received(:unlock) end - - context 'when lock is locked?' do - let(:locked?) { true } - - it 'logs a helpful error message' do - expect { execute }.to raise_error('HELL') - - expect(lock).to have_received(:log_fatal).with(error_message) - end - end end end diff --git a/spec/support/shared_examples/with_a_stubbed_locksmith.rb b/spec/support/shared_examples/with_a_stubbed_locksmith.rb index dec908316..2c73bd185 100644 --- a/spec/support/shared_examples/with_a_stubbed_locksmith.rb +++ b/spec/support/shared_examples/with_a_stubbed_locksmith.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true RSpec.shared_context 'with a stubbed locksmith' do - let(:lock) { described_class.new(item) } let(:locksmith) { instance_double(SidekiqUniqueJobs::Locksmith) } let(:redis_pool) { nil } diff --git a/spec/unit/sidekiq_unique_jobs/lock/base_lock_spec.rb b/spec/unit/sidekiq_unique_jobs/lock/base_lock_spec.rb index 0387762ec..f7a62132d 100644 --- a/spec/unit/sidekiq_unique_jobs/lock/base_lock_spec.rb +++ b/spec/unit/sidekiq_unique_jobs/lock/base_lock_spec.rb @@ -4,7 +4,8 @@ RSpec.describe SidekiqUniqueJobs::Lock::BaseLock do include_context 'with a stubbed locksmith' - + let(:lock) { described_class.new(item, callback) } + let(:callback) { -> {} } let(:item) do { 'jid' => 'maaaahjid', @@ -37,26 +38,27 @@ describe '#execute' do it do - expect { lock.execute(nil) } + expect { lock.execute } .to raise_error(NotImplementedError, "#execute needs to be implemented in #{described_class}") end end describe '#unlock' do - it do - allow(locksmith).to receive(:signal).with(item['jid']).and_return('unlocked') + subject(:unlock) { lock.unlock } - expect(lock.unlock).to eq('unlocked') + before do + allow(locksmith).to receive(:signal).with(item['jid']).and_return('unlocked') end + + it { is_expected.to eq('unlocked') } end describe '#delete' do - it do - allow(locksmith).to receive(:unlock) - allow(locksmith).to receive(:delete).and_return('deleted') + subject { lock.delete } - expect(lock.delete).to eq('deleted') - end + before { allow(locksmith).to receive(:delete).and_return('deleted') } + + it { is_expected.to eq('deleted') } end describe '#locked?' do diff --git a/spec/unit/sidekiq_unique_jobs/lock/until_and_while_executing_spec.rb b/spec/unit/sidekiq_unique_jobs/lock/until_and_while_executing_spec.rb index de80d5ed6..b12db5b37 100644 --- a/spec/unit/sidekiq_unique_jobs/lock/until_and_while_executing_spec.rb +++ b/spec/unit/sidekiq_unique_jobs/lock/until_and_while_executing_spec.rb @@ -4,6 +4,8 @@ RSpec.describe SidekiqUniqueJobs::Lock::UntilAndWhileExecuting do include_context 'with a stubbed locksmith' + let(:lock) { described_class.new(item, callback) } + let(:callback) { -> {} } let(:item) do { 'jid' => 'maaaahjid', @@ -12,7 +14,6 @@ 'args' => ['one'], } end - let(:callback) { -> {} } describe '#execute' do let(:runtime_lock) { instance_spy(SidekiqUniqueJobs::Lock::WhileExecuting) } @@ -21,7 +22,7 @@ allow(lock).to receive(:locked?).and_return(locked?) allow(lock).to receive(:unlock).and_return(true) allow(lock).to receive(:runtime_lock).and_return(runtime_lock) - allow(runtime_lock).to receive(:execute).with(callback).and_yield + allow(runtime_lock).to receive(:execute).and_yield end context 'when locked?' do @@ -30,12 +31,12 @@ it 'unlocks the unique key before yielding' do inside_block_value = false - lock.execute(callback) { inside_block_value = true } + lock.execute { inside_block_value = true } expect(inside_block_value).to eq(true) expect(lock).to have_received(:locked?) expect(lock).to have_received(:unlock) - expect(runtime_lock).to have_received(:execute).with(callback) + expect(runtime_lock).to have_received(:execute) end end @@ -44,12 +45,12 @@ it 'unlocks the unique key before yielding' do inside_block_value = false - lock.execute(callback) { inside_block_value = true } + lock.execute { inside_block_value = true } expect(inside_block_value).to eq(false) expect(lock).to have_received(:locked?) expect(lock).not_to have_received(:unlock) - expect(runtime_lock).not_to have_received(:execute).with(callback) + expect(runtime_lock).not_to have_received(:execute) end end end @@ -65,7 +66,7 @@ expect(SidekiqUniqueJobs::Lock::WhileExecuting) .to have_received(:new) - .with(item, redis_pool) + .with(item, callback, redis_pool) end end end diff --git a/spec/unit/sidekiq_unique_jobs/lock/until_executed_spec.rb b/spec/unit/sidekiq_unique_jobs/lock/until_executed_spec.rb index 5fcfec16f..04d535883 100644 --- a/spec/unit/sidekiq_unique_jobs/lock/until_executed_spec.rb +++ b/spec/unit/sidekiq_unique_jobs/lock/until_executed_spec.rb @@ -4,6 +4,8 @@ RSpec.describe SidekiqUniqueJobs::Lock::UntilExecuted do include_context 'with a stubbed locksmith' + let(:lock) { described_class.new(item, callback) } + let(:callback) { -> {} } let(:item) do { 'jid' => 'maaaahjid', @@ -21,7 +23,7 @@ it 'returns without yielding' do execute - expect(empty_callback).not_to have_received(:call) + expect(callback).not_to have_received(:call) expect(block).not_to have_received(:call) end end @@ -33,18 +35,22 @@ it 'calls back' do expect { execute }.to raise_error('HELL') - expect(empty_callback).to have_received(:call) + expect(callback).to have_received(:call) end end context 'when callback raises error' do - let(:empty_callback) { -> { raise 'CallbackError' } } - let(:locked?) { false } + let(:callback) { -> { raise 'CallbackError' } } + let(:locked?) { false } it 'logs a warning' do expect { execute }.to raise_error('CallbackError') - expect(lock).to have_received(:log_warn).with("the callback for unique_key: #{item['unique_digest']} failed!") + expect(lock).to have_received(:log_warn) + .with( + 'The lock for uniquejobs:1b9f2f0624489ccf4e07ac88beae6ce0' \ + ' has been released but the #after_unlock callback failed!', + ) end end end diff --git a/spec/unit/sidekiq_unique_jobs/lock/until_executing_spec.rb b/spec/unit/sidekiq_unique_jobs/lock/until_executing_spec.rb index 2895cd54b..565d5af15 100644 --- a/spec/unit/sidekiq_unique_jobs/lock/until_executing_spec.rb +++ b/spec/unit/sidekiq_unique_jobs/lock/until_executing_spec.rb @@ -4,19 +4,20 @@ RSpec.describe SidekiqUniqueJobs::Lock::UntilExecuting do include_context 'with a stubbed locksmith' - + let(:lock) { described_class.new(item, callback) } + let(:callback) { -> {} } let(:item) do { 'jid' => 'maaaahjid', 'class' => 'UntilExpiredJob', 'unique' => 'until_timeout' } end - let(:empty_callback) { -> {} } describe '#execute' do it 'calls the callback' do - expect(lock).to receive(:unlock).ordered - expect(empty_callback).to receive(:call) - expect { |block| lock.execute(empty_callback, &block) }.to yield_control + allow(lock).to receive(:unlock_with_callback) + + expect { |block| lock.execute(&block) }.to yield_control + expect(lock).to have_received(:unlock_with_callback) end end end diff --git a/spec/unit/sidekiq_unique_jobs/lock/until_expired_spec.rb b/spec/unit/sidekiq_unique_jobs/lock/until_expired_spec.rb index c9ea0edeb..2ab222847 100644 --- a/spec/unit/sidekiq_unique_jobs/lock/until_expired_spec.rb +++ b/spec/unit/sidekiq_unique_jobs/lock/until_expired_spec.rb @@ -4,13 +4,13 @@ RSpec.describe SidekiqUniqueJobs::Lock::UntilExpired do include_context 'with a stubbed locksmith' - + let(:lock) { described_class.new(item, callback) } + let(:callback) { -> {} } let(:item) do { 'jid' => 'maaaahjid', 'class' => 'UntilExpiredJob', 'unique' => 'until_timeout' } end - let(:empty_callback) { -> {} } describe '#unlock' do subject(:unlock) { lock.unlock } @@ -19,11 +19,11 @@ end before do - allow(empty_callback).to receive(:call) + allow(callback).to receive(:call) end describe '#execute' do - subject(:execute) { lock.execute(empty_callback) } + subject(:execute) { lock.execute(&block) } let(:locked?) { false } @@ -34,18 +34,14 @@ context 'when locked?' do let(:locked?) { true } - it 'calls back' do - execute - - expect(empty_callback).to have_received(:call) + it 'yields to caller' do + expect { |block| lock.execute(&block) }.to yield_control end end context 'when not locked?' do - it 'does not call back' do - execute - - expect(empty_callback).not_to have_received(:call) + it 'does not yield to caller' do + expect { |block| lock.execute(&block) }.not_to yield_control end end end diff --git a/spec/unit/sidekiq_unique_jobs/lock/while_executing_reject_spec.rb b/spec/unit/sidekiq_unique_jobs/lock/while_executing_reject_spec.rb index 74d3a7fa2..cac1d7f64 100644 --- a/spec/unit/sidekiq_unique_jobs/lock/while_executing_reject_spec.rb +++ b/spec/unit/sidekiq_unique_jobs/lock/while_executing_reject_spec.rb @@ -4,16 +4,16 @@ RSpec.describe SidekiqUniqueJobs::Lock::WhileExecutingReject do include_context 'with a stubbed locksmith' - + let(:lock) { described_class.new(item, callback) } + let(:callback) { -> {} } + let(:deadset) { instance_spy(Sidekiq::DeadSet) } + let(:payload) { instance_spy('payload') } let(:item) do { 'jid' => 'maaaahjid', 'class' => 'WhileExecutingRejectJob', 'unique' => 'while_executing_reject', 'args' => [%w[array of arguments]] } end - let(:empty_callback) { -> {} } - let(:deadset) { instance_spy(Sidekiq::DeadSet) } - let(:payload) { instance_spy('payload') } before do allow(lock).to receive(:deadset).and_return(deadset) @@ -28,13 +28,13 @@ end describe '#execute' do - subject(:execute) { lock.execute(empty_callback) } + subject(:execute) { lock.execute } let(:token) { nil } before do allow(locksmith).to receive(:lock).with(0).and_return(token) - allow(lock).to receive(:using_protection).with(empty_callback).and_yield + allow(lock).to receive(:with_cleanup).and_yield end context 'when lock succeeds' do @@ -42,7 +42,7 @@ it 'processes the job' do execute - expect(lock).to have_received(:using_protection) + expect(lock).to have_received(:with_cleanup) end end @@ -53,7 +53,7 @@ expect(lock).to receive(:reject) execute - expect(lock).not_to have_received(:using_protection) + expect(lock).not_to have_received(:with_cleanup) end end end diff --git a/spec/unit/sidekiq_unique_jobs/lock/while_executing_spec.rb b/spec/unit/sidekiq_unique_jobs/lock/while_executing_spec.rb index 3fcb6d05c..ba73f6a54 100644 --- a/spec/unit/sidekiq_unique_jobs/lock/while_executing_spec.rb +++ b/spec/unit/sidekiq_unique_jobs/lock/while_executing_spec.rb @@ -4,6 +4,8 @@ RSpec.describe SidekiqUniqueJobs::Lock::WhileExecuting do include_context 'with a stubbed locksmith' + let(:lock) { described_class.new(item, callback) } + let(:callback) { -> {} } let(:item) do { 'jid' => 'maaaahjid', @@ -11,11 +13,10 @@ 'unique' => 'while_executing', 'args' => [%w[array of arguments]] } end - let(:empty_callback) { -> {} } describe '.new' do specify do - expect { described_class.new(item) } + expect { described_class.new(item, callback) } .to change { item['unique_digest'] } .to a_string_ending_with(':RUN') end @@ -28,7 +29,7 @@ end describe '#execute' do - subject(:execute) { lock.execute(empty_callback) } + subject(:execute) { lock.execute } before do allow(locksmith).to receive(:lock).with(0).and_return(token) @@ -44,7 +45,7 @@ let(:token) { nil } it do - expect { |block| lock.execute(empty_callback, &block) } + expect { |block| lock.execute(&block) } .not_to yield_control end end diff --git a/spec/unit/sidekiq_unique_jobs/server/middleware_spec.rb b/spec/unit/sidekiq_unique_jobs/server/middleware_spec.rb index 2608e4a9c..82acfb037 100644 --- a/spec/unit/sidekiq_unique_jobs/server/middleware_spec.rb +++ b/spec/unit/sidekiq_unique_jobs/server/middleware_spec.rb @@ -24,7 +24,7 @@ before do @inside_block_value = false allow(middleware).to receive(:lock).and_return(lock) - allow(lock).to receive(:execute).with(instance_of(Proc)).and_yield + allow(lock).to receive(:execute).and_yield end context 'when unique is disabled' do