From cf69746b1bede2ab2bdb36f4f810ed2719d38a53 Mon Sep 17 00:00:00 2001 From: Mikael Henriksson Date: Sat, 30 Jun 2018 12:59:27 +0200 Subject: [PATCH 01/11] Improve README --- README.md | 106 ++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 68 insertions(+), 38 deletions(-) diff --git a/README.md b/README.md index 7ff394619..43e218bc2 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. - -#### NOTE: +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. -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,7 +235,6 @@ 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. @@ -222,12 +249,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 +267,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 From f9b9bcd83684cbf1b4e5ddb2415c53ed90d91c9c Mon Sep 17 00:00:00 2001 From: Mikael Henriksson Date: Sat, 30 Jun 2018 13:02:15 +0200 Subject: [PATCH 02/11] Prefer unique_across_queues for consistency --- lib/sidekiq_unique_jobs/constants.rb | 1 + lib/sidekiq_unique_jobs/unique_args.rb | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/sidekiq_unique_jobs/constants.rb b/lib/sidekiq_unique_jobs/constants.rb index ea616f805..76b28893f 100644 --- a/lib/sidekiq_unique_jobs/constants.rb +++ b/lib/sidekiq_unique_jobs/constants.rb @@ -9,6 +9,7 @@ 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' 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? From 584ba6f9785933206ea017f9229b157509c83174 Mon Sep 17 00:00:00 2001 From: Mikael Henriksson Date: Sat, 30 Jun 2018 15:59:05 +0200 Subject: [PATCH 03/11] Improve bug report --- .github/ISSUE_TEMPLATE/bug_report.md | 35 ++++++++++++---------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md index b73537336..bc568e3d8 100644 --- a/.github/ISSUE_TEMPLATE/bug_report.md +++ b/.github/ISSUE_TEMPLATE/bug_report.md @@ -7,29 +7,24 @@ 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. - -**Desktop (please complete the following information):** - - OS: [e.g. iOS] - - Browser [e.g. chrome, safari] - - Version [e.g. 22] - -**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] +**Current behavior** +What happens instead of the expected behavior? + +**Worker class** +```ruby +class MyWorker + include Sidekiq::Worker + sidekiq_options unique: :until_executed, queue: :undefault + def perform(args); end + + def self.unique_args(args) + # the way you consider unique arguments + end +end +``` **Additional context** Add any other context about the problem here. From 8b7178773e2adaf8739a587828fe834aed817a55 Mon Sep 17 00:00:00 2001 From: Mikael Henriksson Date: Sat, 30 Jun 2018 16:00:07 +0200 Subject: [PATCH 04/11] Unlock is already done in base class --- lib/sidekiq_unique_jobs/lock/until_executed.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/sidekiq_unique_jobs/lock/until_executed.rb b/lib/sidekiq_unique_jobs/lock/until_executed.rb index be00b0932..7ec789ee4 100644 --- a/lib/sidekiq_unique_jobs/lock/until_executed.rb +++ b/lib/sidekiq_unique_jobs/lock/until_executed.rb @@ -10,7 +10,6 @@ def execute(callback) using_protection(callback) do yield if block_given? end - unlock end end end From c13b2a3c7aa382b64ccedf14b6c331a86f3ee0d2 Mon Sep 17 00:00:00 2001 From: Mikael Henriksson Date: Sat, 30 Jun 2018 16:00:38 +0200 Subject: [PATCH 05/11] Add TODO item to remove constant in v6.1 --- lib/sidekiq_unique_jobs/constants.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/sidekiq_unique_jobs/constants.rb b/lib/sidekiq_unique_jobs/constants.rb index 76b28893f..b312f3000 100644 --- a/lib/sidekiq_unique_jobs/constants.rb +++ b/lib/sidekiq_unique_jobs/constants.rb @@ -14,6 +14,6 @@ module SidekiqUniqueJobs 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 From ccb18428a1bde262af78023e25f00b636d39d355 Mon Sep 17 00:00:00 2001 From: Mikael Henriksson Date: Sat, 30 Jun 2018 16:02:00 +0200 Subject: [PATCH 06/11] Ignore some files in git --- .gitignore | 4 ++++ 1 file changed, 4 insertions(+) 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/ From d26d7c2cc1e9799b59cfeb4b820252ca42070763 Mon Sep 17 00:00:00 2001 From: Mikael Henriksson Date: Sat, 30 Jun 2018 17:01:04 +0200 Subject: [PATCH 07/11] Clarify after_unlock --- README.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 43e218bc2..c3a788265 100644 --- a/README.md +++ b/README.md @@ -237,7 +237,10 @@ 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 From 2137420475cfa490798812721d79cab35958abdd Mon Sep 17 00:00:00 2001 From: Mikael Henriksson Date: Sat, 30 Jun 2018 17:02:33 +0200 Subject: [PATCH 08/11] Simplify dealing with callbacks Instead of duplicating the argument everywhere just initialize the lock with the callback. --- lib/sidekiq_unique_jobs/lock/base_lock.rb | 39 +++++++++++-------- .../lock/until_and_while_executing.rb | 6 +-- .../lock/until_executed.rb | 6 +-- .../lock/until_executing.rb | 5 +-- lib/sidekiq_unique_jobs/lock/until_expired.rb | 4 +- .../lock/while_executing.rb | 13 +++---- .../lock/while_executing_reject.rb | 8 +--- .../lock/while_executing_requeue.rb | 7 ++-- .../options_with_fallback.rb | 2 +- lib/sidekiq_unique_jobs/server/middleware.rb | 6 +-- .../sidekiq_worker_methods.rb | 4 ++ .../lock/until_and_while_executing_spec.rb | 34 ++++++++-------- .../lock/until_executed_spec.rb | 10 ++--- .../lock/until_expired_spec.rb | 26 +++---------- .../lock/while_executing_reject_spec.rb | 10 ++--- .../lock/while_executing_spec.rb | 12 +++--- .../an_executing_lock_with_error_handling.rb | 32 +-------------- .../with_a_stubbed_locksmith.rb | 1 - .../lock/base_lock_spec.rb | 37 +++++++++++++----- .../lock/until_and_while_executing_spec.rb | 15 +++---- .../lock/until_executed_spec.rb | 12 +++--- .../lock/until_executing_spec.rb | 11 +++--- .../lock/until_expired_spec.rb | 20 ++++------ .../lock/while_executing_reject_spec.rb | 16 ++++---- .../lock/while_executing_spec.rb | 9 +++-- .../server/middleware_spec.rb | 2 +- 26 files changed, 158 insertions(+), 189 deletions(-) diff --git a/lib/sidekiq_unique_jobs/lock/base_lock.rb b/lib/sidekiq_unique_jobs/lock/base_lock.rb index 012a58b45..616a0f9c3 100644 --- a/lib/sidekiq_unique_jobs/lock/base_lock.rb +++ b/lib/sidekiq_unique_jobs/lock/base_lock.rb @@ -5,28 +5,33 @@ 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 def unlock + return notify_about_manual_unlock unless operative locksmith.signal(item[JID_KEY]) # Only signal to release the lock end def delete + return notify_about_manual_unlock unless operative locksmith.delete # Soft delete (don't forcefully remove when expiration is set) end def delete! + return notify_about_manual_unlock unless operative locksmith.delete! # Force delete the lock end @@ -36,20 +41,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 +64,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 + token = unlock + return notify_about_manual_unlock unless token + callback_safely + + token 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 7ec789ee4..878f032a3 100644 --- a/lib/sidekiq_unique_jobs/lock/until_executed.rb +++ b/lib/sidekiq_unique_jobs/lock/until_executed.rb @@ -5,11 +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 + 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/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..cf4950511 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,28 +38,44 @@ 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 + subject(:unlock) { lock.unlock } + + before do + allow(lock).to receive(:operative).and_return(operative) allow(locksmith).to receive(:signal).with(item['jid']).and_return('unlocked') + allow(lock).to receive(:notify_about_manual_unlock) + end + + context 'when operative' do + let(:operative) { true } - expect(lock.unlock).to eq('unlocked') + it { is_expected.to eq('unlocked') } end - end - describe '#delete' do - it do - allow(locksmith).to receive(:unlock) - allow(locksmith).to receive(:delete).and_return('deleted') + context 'when inoperative' do + let(:operative) { false } + + it { is_expected.to eq(nil) } - expect(lock.delete).to eq('deleted') + it 'notifies about manual unlock' do + unlock + expect(lock).to have_received(:notify_about_manual_unlock) + end end end + describe '#delete' do + subject { lock.delete } + before { allow(locksmith).to receive(:delete).and_return('deleted') } + it { is_expected.to eq('deleted') } + end + describe '#locked?' do it do allow(locksmith).to receive(:locked?).and_return(true) 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..ff0870f1e 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,18 @@ 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 From ecd3ecb8763c4803740140c7dbf102dddc80f4c8 Mon Sep 17 00:00:00 2001 From: Mikael Henriksson Date: Sat, 30 Jun 2018 17:21:16 +0200 Subject: [PATCH 09/11] =?UTF-8?q?Rubo=F0=9F=91=AE?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- spec/unit/sidekiq_unique_jobs/lock/base_lock_spec.rb | 2 ++ spec/unit/sidekiq_unique_jobs/lock/until_executed_spec.rb | 6 +++++- 2 files changed, 7 insertions(+), 1 deletion(-) 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 cf4950511..ed92bdaed 100644 --- a/spec/unit/sidekiq_unique_jobs/lock/base_lock_spec.rb +++ b/spec/unit/sidekiq_unique_jobs/lock/base_lock_spec.rb @@ -72,7 +72,9 @@ describe '#delete' do subject { lock.delete } + before { allow(locksmith).to receive(:delete).and_return('deleted') } + it { is_expected.to eq('deleted') } 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 ff0870f1e..04d535883 100644 --- a/spec/unit/sidekiq_unique_jobs/lock/until_executed_spec.rb +++ b/spec/unit/sidekiq_unique_jobs/lock/until_executed_spec.rb @@ -46,7 +46,11 @@ it 'logs a warning' do expect { execute }.to raise_error('CallbackError') - expect(lock).to have_received(:log_warn).with('The lock for uniquejobs:1b9f2f0624489ccf4e07ac88beae6ce0 has been released but the #after_unlock callback 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 From 899baa6703f2534a46873ae9c716493bb8c3fdf0 Mon Sep 17 00:00:00 2001 From: Mikael Henriksson Date: Sat, 30 Jun 2018 17:25:59 +0200 Subject: [PATCH 10/11] Let's forget about operative for unlock --- lib/sidekiq_unique_jobs/lock/base_lock.rb | 11 ++++------- .../lock/base_lock_spec.rb | 19 +------------------ 2 files changed, 5 insertions(+), 25 deletions(-) diff --git a/lib/sidekiq_unique_jobs/lock/base_lock.rb b/lib/sidekiq_unique_jobs/lock/base_lock.rb index 616a0f9c3..af27b3399 100644 --- a/lib/sidekiq_unique_jobs/lock/base_lock.rb +++ b/lib/sidekiq_unique_jobs/lock/base_lock.rb @@ -21,17 +21,14 @@ def execute end def unlock - return notify_about_manual_unlock unless operative locksmith.signal(item[JID_KEY]) # Only signal to release the lock end def delete - return notify_about_manual_unlock unless operative locksmith.delete # Soft delete (don't forcefully remove when expiration is set) end def delete! - return notify_about_manual_unlock unless operative locksmith.delete! # Force delete the lock end @@ -70,11 +67,11 @@ def notify_about_manual_unlock end def unlock_with_callback - token = unlock - return notify_about_manual_unlock unless token - callback_safely + return notify_about_manual_unlock unless operative + return notify_about_manual_unlock unless unlock - token + callback_safely + item[JID_KEY] end def callback_safely 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 ed92bdaed..f7a62132d 100644 --- a/spec/unit/sidekiq_unique_jobs/lock/base_lock_spec.rb +++ b/spec/unit/sidekiq_unique_jobs/lock/base_lock_spec.rb @@ -47,27 +47,10 @@ subject(:unlock) { lock.unlock } before do - allow(lock).to receive(:operative).and_return(operative) allow(locksmith).to receive(:signal).with(item['jid']).and_return('unlocked') - allow(lock).to receive(:notify_about_manual_unlock) end - context 'when operative' do - let(:operative) { true } - - it { is_expected.to eq('unlocked') } - end - - context 'when inoperative' do - let(:operative) { false } - - it { is_expected.to eq(nil) } - - it 'notifies about manual unlock' do - unlock - expect(lock).to have_received(:notify_about_manual_unlock) - end - end + it { is_expected.to eq('unlocked') } end describe '#delete' do From d36f758614ebae32d229900078fc5ddfdf10ffeb Mon Sep 17 00:00:00 2001 From: Mikael Henriksson Date: Sat, 30 Jun 2018 17:28:27 +0200 Subject: [PATCH 11/11] Styles --- .github/ISSUE_TEMPLATE/bug_report.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md index bc568e3d8..d6d0bf549 100644 --- a/.github/ISSUE_TEMPLATE/bug_report.md +++ b/.github/ISSUE_TEMPLATE/bug_report.md @@ -14,6 +14,7 @@ A clear and concise description of what you expected to happen. What happens instead of the expected behavior? **Worker class** + ```ruby class MyWorker include Sidekiq::Worker