From 88ed7a96146799d5b01a550a7cccc937045c419c Mon Sep 17 00:00:00 2001 From: Mika Hel Date: Sat, 9 Sep 2017 18:19:05 +0200 Subject: [PATCH] Better runtime locks (#241) * Improve locking/unlocking and testing * Avoid crashing the spec suite for pry * Fix a couple of flaky tests * Make sure this spec runs when pry is present * Make mock_redis available to all gemfiles --- .gitignore | 2 + .rubocop.yml | 4 + .simplecov | 3 +- .travis.yml | 45 ++- Appraisals | 5 + CHANGELOG.md | 4 + Gemfile | 4 +- README.md | 78 ++-- gemfiles/sidekiq_4.0.gemfile | 25 +- gemfiles/sidekiq_4.1.gemfile | 25 +- gemfiles/sidekiq_4.2.gemfile | 25 +- gemfiles/sidekiq_5.0.gemfile | 25 +- gemfiles/sidekiq_develop.gemfile | 27 +- lib/sidekiq-unique-jobs.rb | 6 +- lib/sidekiq_unique_jobs/cli.rb | 7 - lib/sidekiq_unique_jobs/client/middleware.rb | 7 +- lib/sidekiq_unique_jobs/constants.rb | 13 +- lib/sidekiq_unique_jobs/exceptions.rb | 24 ++ lib/sidekiq_unique_jobs/lock.rb | 230 +++++++++++- .../lock/prepares_items.rb | 14 + .../lock/queue_lock_base.rb | 35 ++ .../lock/reschedule_while_executing.rb | 24 ++ lib/sidekiq_unique_jobs/lock/run_lock_base.rb | 37 ++ .../lock/until_and_while_executing.rb | 36 +- .../lock/until_executed.rb | 49 +-- .../lock/until_executing.rb | 35 +- lib/sidekiq_unique_jobs/lock/until_timeout.rb | 28 +- .../lock/while_executing.rb | 63 ++-- .../options_with_fallback.rb | 30 +- lib/sidekiq_unique_jobs/run_lock_failed.rb | 3 - lib/sidekiq_unique_jobs/script_mock.rb | 69 ---- lib/sidekiq_unique_jobs/scripts.rb | 10 +- .../scripts/acquire_lock.rb | 47 --- .../scripts/release_lock.rb | 49 --- lib/sidekiq_unique_jobs/server/middleware.rb | 16 +- lib/sidekiq_unique_jobs/sidekiq_unique_ext.rb | 22 +- lib/sidekiq_unique_jobs/testing.rb | 87 +++-- .../testing/sidekiq_overrides.rb | 14 +- lib/sidekiq_unique_jobs/timeout.rb | 10 + lib/sidekiq_unique_jobs/timeout/calculator.rb | 51 +++ lib/sidekiq_unique_jobs/timeout/queue_lock.rb | 15 + lib/sidekiq_unique_jobs/timeout/run_lock.rb | 14 + lib/sidekiq_unique_jobs/timeout_calculator.rb | 67 ---- lib/sidekiq_unique_jobs/unlockable.rb | 21 +- lib/sidekiq_unique_jobs/util.rb | 71 +--- rails_example/Gemfile | 5 +- .../app/channels/appearance_channel.rb | 4 +- .../app/controllers/work_controller.rb | 13 + .../app/views/layouts/application.html.erb | 15 - .../app/views/layouts/application.html.slim | 25 ++ rails_example/app/views/work/index.html.slim | 0 .../while_executing_with_timeout_worker.rb | 14 + rails_example/config/routes.rb | 15 +- .../controllers/work_controller_mock_spec.rb | 36 +- .../spec/controllers/work_controller_spec.rb | 6 +- rails_example/spec/rails_helper.rb | 1 + rails_example/spec/support/sidekiq_meta.rb | 14 - redis/acquire_lock.lua | 19 - redis/exists_or_create.lua | 29 ++ redis/release_lock.lua | 16 - redis/release_stale_locks.lua | 89 +++++ redis/synchronize.lua | 16 - sidekiq-unique-jobs.gemspec | 4 +- spec/integration/client/middleware_spec.rb | 333 +++++++++++++++++ spec/integration/sidekiq_testing_fake_spec.rb | 131 +++++++ .../sidekiq_testing_inline_spec.rb | 47 +++ spec/jobs/custom_queue_job.rb | 4 +- spec/jobs/expiring_job.rb | 6 +- spec/jobs/inline_worker.rb | 6 +- spec/jobs/just_a_worker.rb | 4 +- spec/jobs/long_running_job.rb | 2 +- .../long_running_run_lock_expiration_job.rb | 8 + spec/jobs/my_job.rb | 2 +- spec/jobs/my_unique_job.rb | 14 +- spec/jobs/plain_class.rb | 8 +- spec/jobs/test_class.rb | 4 +- spec/jobs/until_and_while_executing_job.rb | 6 +- spec/jobs/until_executed_2_job.rb | 22 ++ spec/jobs/until_executed_job.rb | 7 +- spec/jobs/until_timeout_job.rb | 8 +- spec/jobs/while_executing_job.rb | 2 +- .../client/middleware_spec.rb | 345 ------------------ .../lock/until_executed_spec.rb | 39 -- .../lock/while_executing_spec.rb | 88 ----- .../queue_lock_timeout_calculator_spec.rb | 49 --- .../run_lock_timeout_calculator_spec.rb | 44 --- .../sidekiq_unique_jobs/script_mock_spec.rb | 109 ------ .../scripts/acquire_lock_spec.rb | 52 --- .../scripts/release_lock_spec.rb | 43 --- .../server/middleware_spec.rb | 80 ---- .../sidekiq_testing_enabled_spec.rb | 166 --------- .../sidekiq_unique_jobs/unlockable_spec.rb | 58 --- spec/lib/sidekiq_unique_jobs/util_spec.rb | 84 ----- spec/spec_helper.rb | 44 +-- spec/support/matchers/redis_matchers.rb | 11 +- spec/support/sidekiq_meta.rb | 56 ++- .../sidekiq_unique_jobs => unit}/cli_spec.rb | 81 +--- spec/unit/client/middleware_spec.rb | 29 ++ .../config_spec.rb | 0 .../core_ext_spec.rb | 0 .../lock/until_and_while_executing_spec.rb | 26 +- spec/unit/lock/until_executed_spec.rb | 43 +++ .../lock/until_timeout_spec.rb | 15 +- spec/unit/lock/while_executing_spec.rb | 6 + spec/unit/lock_spec.rb | 250 +++++++++++++ .../normalizer_spec.rb | 8 +- .../options_with_fallback_spec.rb | 0 .../scripts_spec.rb | 4 +- spec/unit/server/middleware_spec.rb | 95 +++++ .../sidekiq_unique_ext_spec.rb | 48 ++- .../sidekiq_unique_jobs_spec.rb | 0 .../timeout/calculator_spec.rb} | 20 +- spec/unit/timeout/queue_lock_spec.rb | 97 +++++ spec/unit/timeout/run_lock_spec.rb | 58 +++ .../unique_args_spec.rb | 0 spec/unit/unlockable_spec.rb | 51 +++ spec/unit/util_spec.rb | 96 +++++ 117 files changed, 2506 insertions(+), 2100 deletions(-) create mode 100644 lib/sidekiq_unique_jobs/exceptions.rb create mode 100644 lib/sidekiq_unique_jobs/lock/prepares_items.rb create mode 100644 lib/sidekiq_unique_jobs/lock/queue_lock_base.rb create mode 100644 lib/sidekiq_unique_jobs/lock/reschedule_while_executing.rb create mode 100644 lib/sidekiq_unique_jobs/lock/run_lock_base.rb delete mode 100644 lib/sidekiq_unique_jobs/run_lock_failed.rb delete mode 100644 lib/sidekiq_unique_jobs/script_mock.rb delete mode 100644 lib/sidekiq_unique_jobs/scripts/acquire_lock.rb delete mode 100644 lib/sidekiq_unique_jobs/scripts/release_lock.rb create mode 100644 lib/sidekiq_unique_jobs/timeout.rb create mode 100644 lib/sidekiq_unique_jobs/timeout/calculator.rb create mode 100644 lib/sidekiq_unique_jobs/timeout/queue_lock.rb create mode 100644 lib/sidekiq_unique_jobs/timeout/run_lock.rb delete mode 100644 lib/sidekiq_unique_jobs/timeout_calculator.rb delete mode 100644 rails_example/app/views/layouts/application.html.erb create mode 100644 rails_example/app/views/layouts/application.html.slim create mode 100644 rails_example/app/views/work/index.html.slim create mode 100644 rails_example/app/workers/while_executing_with_timeout_worker.rb delete mode 100644 rails_example/spec/support/sidekiq_meta.rb delete mode 100644 redis/acquire_lock.lua create mode 100644 redis/exists_or_create.lua delete mode 100644 redis/release_lock.lua create mode 100644 redis/release_stale_locks.lua delete mode 100644 redis/synchronize.lua create mode 100644 spec/integration/client/middleware_spec.rb create mode 100644 spec/integration/sidekiq_testing_fake_spec.rb create mode 100644 spec/integration/sidekiq_testing_inline_spec.rb create mode 100644 spec/jobs/long_running_run_lock_expiration_job.rb create mode 100644 spec/jobs/until_executed_2_job.rb delete mode 100644 spec/lib/sidekiq_unique_jobs/client/middleware_spec.rb delete mode 100644 spec/lib/sidekiq_unique_jobs/lock/until_executed_spec.rb delete mode 100644 spec/lib/sidekiq_unique_jobs/lock/while_executing_spec.rb delete mode 100644 spec/lib/sidekiq_unique_jobs/queue_lock_timeout_calculator_spec.rb delete mode 100644 spec/lib/sidekiq_unique_jobs/run_lock_timeout_calculator_spec.rb delete mode 100644 spec/lib/sidekiq_unique_jobs/script_mock_spec.rb delete mode 100644 spec/lib/sidekiq_unique_jobs/scripts/acquire_lock_spec.rb delete mode 100644 spec/lib/sidekiq_unique_jobs/scripts/release_lock_spec.rb delete mode 100644 spec/lib/sidekiq_unique_jobs/server/middleware_spec.rb delete mode 100644 spec/lib/sidekiq_unique_jobs/sidekiq_testing_enabled_spec.rb delete mode 100644 spec/lib/sidekiq_unique_jobs/unlockable_spec.rb delete mode 100644 spec/lib/sidekiq_unique_jobs/util_spec.rb rename spec/{lib/sidekiq_unique_jobs => unit}/cli_spec.rb (62%) create mode 100644 spec/unit/client/middleware_spec.rb rename spec/{lib/sidekiq_unique_jobs => unit}/config_spec.rb (100%) rename spec/{lib/sidekiq_unique_jobs => unit}/core_ext_spec.rb (100%) rename spec/{lib/sidekiq_unique_jobs => unit}/lock/until_and_while_executing_spec.rb (55%) create mode 100644 spec/unit/lock/until_executed_spec.rb rename spec/{lib/sidekiq_unique_jobs => unit}/lock/until_timeout_spec.rb (66%) create mode 100644 spec/unit/lock/while_executing_spec.rb create mode 100644 spec/unit/lock_spec.rb rename spec/{lib/sidekiq_unique_jobs => unit}/normalizer_spec.rb (71%) rename spec/{lib/sidekiq_unique_jobs => unit}/options_with_fallback_spec.rb (100%) rename spec/{lib/sidekiq_unique_jobs => unit}/scripts_spec.rb (93%) create mode 100644 spec/unit/server/middleware_spec.rb rename spec/{lib/sidekiq_unique_jobs => unit}/sidekiq_unique_ext_spec.rb (54%) rename spec/{lib/sidekiq_unique_jobs => unit}/sidekiq_unique_jobs_spec.rb (100%) rename spec/{lib/sidekiq_unique_jobs/timeout_calculator_spec.rb => unit/timeout/calculator_spec.rb} (82%) create mode 100644 spec/unit/timeout/queue_lock_spec.rb create mode 100644 spec/unit/timeout/run_lock_spec.rb rename spec/{lib/sidekiq_unique_jobs => unit}/unique_args_spec.rb (100%) create mode 100644 spec/unit/unlockable_spec.rb create mode 100644 spec/unit/util_spec.rb diff --git a/.gitignore b/.gitignore index a44881410..e8379cd94 100644 --- a/.gitignore +++ b/.gitignore @@ -16,3 +16,5 @@ rails_example/spec/examples.txt *.sublime-* /sidekiq/ + +/spec/examples.txt diff --git a/.rubocop.yml b/.rubocop.yml index 58ebb2aaa..bd2279dec 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -47,6 +47,10 @@ Style/FrozenStringLiteralComment: Style/Documentation: Enabled: false +Style/SignalException: + EnforcedStyle: only_fail + Enabled: false + Style/FileName: Enabled: true Exclude: diff --git a/.simplecov b/.simplecov index 71312a162..d786c116a 100644 --- a/.simplecov +++ b/.simplecov @@ -1,7 +1,6 @@ require 'simplecov-json' -require 'codeclimate-test-reporter' -SimpleCov.command_name 'rspec' +SimpleCov.command_name 'RSpec' SimpleCov.refuse_coverage_drop SimpleCov.formatters = [ SimpleCov::Formatter::HTMLFormatter, diff --git a/.travis.yml b/.travis.yml index daa7ad98e..3ef157584 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,21 +1,39 @@ -sudo: false +env: + matrix: + - STYLE=false + global: + - CC_TEST_REPORTER_ID=88e524e8f638efe690def7a6e2c72b1a9db5cdfa74548921b734d609a5858ee5 + - GIT_COMMITTED_AT=$(if [ "$TRAVIS_PULL_REQUEST" == "false" ]; then git log -1 --pretty=format:%ct; else git log -1 --skip 1 --pretty=format:%ct; fi) +dist: trusty +sudo: required language: ruby cache: bundler -# before_install: -# - rvm get head -# - gem update --system -# - gem install bundler -services: - - redis-server + + +before_install: + - sudo rm -rf /etc/apt/sources.list.d/rwky-redis.list + - sudo add-apt-repository ppa:chris-lea/redis-server -y + - sudo apt-get update -qy + - sudo apt-get install redis-server + - sudo service redis-server start +before_script: + - curl -L https://codeclimate.com/downloads/test-reporter/test-reporter-latest-linux-amd64 > ./cc-test-reporter + - chmod +x ./cc-test-reporter script: - if [[ "${STYLE}" = "true" ]]; then bundle exec rubocop - P; fi; - bundle exec rspec spec - - if [[ "${STYLE}" = "true" ]]; then CODECLIMATE_REPO_TOKEN=88e524e8f638efe690def7a6e2c72b1a9db5cdfa74548921b734d609a5858ee5 bundle exec codeclimate-test-reporter; fi; + - if [[ "${STYLE}" = "true" ]]; then ./cc-test-reporter after-build --exit-code $TRAVIS_TEST_RESULT; fi; + rvm: - 2.4.1 - 2.3.2 - jruby-9.1.8.0 -env: STYLE=false +matrix: + fast_finish: true + include: + - rvm: 2.4.1 + gemfile: gemfiles/sidekiq_develop.gemfile + env: STYLE=true gemfile: - gemfiles/sidekiq_develop.gemfile - gemfiles/sidekiq_4.0.gemfile @@ -27,12 +45,3 @@ notifications: email: recipients: - mikael@zoolutions.se -matrix: - fast_finish: true - include: - - rvm: 2.4.1 - gemfile: gemfiles/sidekiq_develop.gemfile - env: STYLE=true -addons: - code_climate: - repo_token: 88e524e8f638efe690def7a6e2c72b1a9db5cdfa74548921b734d609a5858ee5 diff --git a/Appraisals b/Appraisals index 7626ac7bc..72f3b58fb 100644 --- a/Appraisals +++ b/Appraisals @@ -1,21 +1,26 @@ # frozen_string_literal: true appraise 'sidekiq-develop' do + gem 'mock_redis' gem 'sidekiq', github: 'mperham/sidekiq' end appraise 'sidekiq-4.0' do + gem 'mock_redis' gem 'sidekiq', '~> 4.0.0' end appraise 'sidekiq-4.1' do + gem 'mock_redis' gem 'sidekiq', '~> 4.1.0' end appraise 'sidekiq-4.2' do + gem 'mock_redis' gem 'sidekiq', '~> 4.2.0' end appraise 'sidekiq-5.0' do + gem 'mock_redis' gem 'sidekiq', '>= 5.0.0.beta', '< 6' end diff --git a/CHANGELOG.md b/CHANGELOG.md index f99cd08d6..758c0ca90 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +## v5.1.0 + +- Refactoring the runtime locks (WhileExecuting) + ## v5.0.10 - Cleans up test setup and make tests more readable diff --git a/Gemfile b/Gemfile index a42bced0b..70dc49fa7 100644 --- a/Gemfile +++ b/Gemfile @@ -3,16 +3,14 @@ source 'https://rubygems.org' gemspec -gem 'appraisal', '~> 2.0.0' +gem 'appraisal', '~> 2.2.0' gem 'rspec-its', require: false gem 'rspec-wait', require: false platforms :mri_24 do gem 'benchmark-ips', require: false - gem 'codeclimate-test-reporter', require: false gem 'fasterer', require: false gem 'memory_profiler', require: false - gem 'mock_redis', require: false gem 'pry-byebug', require: false gem 'rubocop', require: false gem 'simplecov-json', require: false diff --git a/README.md b/README.md index 57194c94b..b3e628c7f 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,6 @@ The missing unique jobs for sidekiq ## 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. ### Version 4 Upgrade instructions @@ -27,21 +26,48 @@ Or install it yourself as: ## Locking -Like @mperham mentions on [this wiki page](https://github.com/mperham/sidekiq/wiki/Related-Projects#unique-jobs) it is hard to enforce uniqueness with redis in a distributed redis setting. +Sidekiq consists of a client and a server. The client is responsible for pushing jobs to the queue and the worker is responsible for popping jobs from the queue. Most of the uniqueness is handled when the client is pushing jobs to the queue. The client checks if it is allowed to put a job on the queue. +This is probably the most common way of locking. -To make things worse there are many ways of wanting to enforce uniqueness. +The server can also lock a job. It does so by creating a lock when it is executing and removing the lock after it is done executing. -### While Executing +### Options + +#### Lock Expiration -Due to discoverability of the different lock types `unique` sidekiq option it was decided to use the `while_executing` as a default. Most people will note that scheduling any number of jobs with the same arguments is possible. +This is the a number in seconds that the lock should be considered unique for. By default the lock doesn't expire at all. + +If you want to experiment with various expirations please provide the following argument: ```ruby -sidekiq_options unique: :while_executing +sidekiq_options lock_expiration: (2 * 60) # 2 minutes +``` + +#### Lock Timeout + +This is the timeout (how long to wait) for creating the lock. By default we don't use a timeout so we won't wait for the lock to be created. If you want it is possible to set this like below. + +```ruby +sidekiq_options lock_timeout: 5 # 5 seconds ``` -Is to make sure that a job can be scheduled any number of times but only executed a single time per argument provided to the job we call this runtime uniqueness. This is probably most useful for background jobs that are fast to execute. (See mhenrixon/sidekiq-unique-jobs#111 for a great example of when this would be right.) While the job is executing/performing no other jobs can be executed at the same time. +#### Lock Resources + +This allows us to perform multiple locks for a unique key. + +```ruby +sidekiq_options lock_resources: 2 # Use 2 locks +``` -The way it currently works is that the jobs can be put on the queue but any succeedent job will wait until the first one finishes. +#### + +### While Executing + +With this lock type it is possible to put any number of these jobs on the queue at 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. + +```ruby +sidekiq_options unique: :while_executing +``` There is an example of this to try it out in the `rails_example` application. Run `foreman start` in the root of the directory and open the url: `localhost:5000/work/duplicate_while_executing`. @@ -64,45 +90,42 @@ In the console you should see something like: ### Until Executing +These jobs will be unique until they have been taken off the queue by the sidekiq server. Then new jobs can be pushed to the queue again. + +**Note:** For slow running jobs this is probably not the best choice as another slow running job with the same arguments could potentially be started. There is nothing that prevents simultaneous jobs to be running. + ```ruby sidekiq_options unique: :until_executing ``` -This means that a job can only be scheduled into redis once per whatever the configuration of unique arguments. Any jobs added until the first one of the same arguments has been unlocked will just be dropped. This is what was tripping many people up. They would schedule a job to run in the future and it would be impossible to schedule new jobs with those same arguments even immediately. There was some forth and back between also locking jobs on the scheduled queue and the regular queues but in the end I decided it was best to separate these two features out into different locking mechanisms. I think what most people are after is to be able to lock a job while executing or that seems to be what people are most missing at the moment. - ### Until Executed +When these jobs are pushed to the queue by the sidekiq client a key is created that won't be removed until the sidekiq server successfully executed the job. + +**Note:** Uniqueness is kept from when the job is pushed to the queue until after it is processed. + ```ruby sidekiq_options unique: :until_executed ``` -This is the combination of the two above. First we lock the job until it executes, then as the job begins executes we keep the lock so that no other jobs with the same arguments can execute at the same time. - ### Until Timeout +These jobs will be unique until they timeout. In the meantime no further jobs will be created with the given unique arguments. + ```ruby sidekiq_options unique: :until_timeout ``` -The job won't be unlocked until the timeout/expiry runs out. - ### Unique Until And While Executing +First a unique key is created when the Sidekiq client pushes the job to the queue. No job with the same arguments can be pushed to the queue. Then as the server pops the job off of the queue the original lock is unlocked and the server then creates a + ```ruby sidekiq_options unique: :until_and_while_executing ``` -This lock is exactly what you would expect. It is considered unique in a way until executing begins and it is locked while executing so what differs from `UntilExecuted`? - -The difference is that this job has two types of uniqueness: -1. It is unique until execution -2. It is unique while executing - -That means it locks for any job with the same arguments to be persisted into redis and just like you would expect it will only ever allow one job of the same unique arguments to run at any given time but as soon as the runtime lock has been acquired the schedule/async lock is released. - ### Uniqueness Scope - - Queue specific locks - Across all queues - [spec/jobs/unique_on_all_queues_job.rb](https://github.com/mhenrixon/sidekiq-unique-jobs/blob/master/spec/jobs/unique_on_all_queues_job.rb) - Across all workers - [spec/jobs/unique_across_workers_job.rb](https://github.com/mhenrixon/sidekiq-unique-jobs/blob/master/spec/jobs/unique_across_workers_job.rb) @@ -123,16 +146,16 @@ or until the job has been completed. Thus, the job will be unique for the shorte If you want the unique job to stick around even after it has been successfully processed then just set `unique: :until_timeout`. -You can also control the expiration length of the uniqueness check. If you want to enforce uniqueness over a longer period than the default of 30 minutes then you can pass the number of seconds you want to use to the sidekiq options: +You can also control the `lock_expiration` of the uniqueness check. If you want to enforce uniqueness over a longer period than the default of 30 minutes then you can pass the number of seconds you want to use to the sidekiq options: ```ruby -sidekiq_options unique: :until_timeout, unique_expiration: 120 * 60 # 2 hours +sidekiq_options unique: :until_timeout, lock_expiration: 120 * 60 # 2 hours ``` For locking modes (`:while_executing` and `:until_and_while_executing`) you can control the expiration length of the runtime uniqueness. If you want to enforce uniqueness over a longer period than the default of 60 seconds, then you can pass the number of seconds you want to use to the sidekiq options: ```ruby -sidekiq_options unique: :while_executing, run_lock_expiration: 2 * 60 # 2 minutes +sidekiq_options unique: :while_executing, lock_expiration: 2 * 60 # 2 minutes ``` Requiring the gem in your gemfile should be sufficient to enable unique jobs. @@ -246,9 +269,6 @@ Start the console with the following command `bundle exec jobs console`. #### 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. -#### Expire -`expire` clears the unique hash from expired keys - ### Command Line `bundle exec jobs` displays help on how to use the unique jobs command line. diff --git a/gemfiles/sidekiq_4.0.gemfile b/gemfiles/sidekiq_4.0.gemfile index 1a5747a87..96612e7c2 100644 --- a/gemfiles/sidekiq_4.0.gemfile +++ b/gemfiles/sidekiq_4.0.gemfile @@ -2,21 +2,20 @@ source "https://rubygems.org" -gem "appraisal", "~> 2.0.0" -gem "rspec-its", :require => false -gem "rspec-wait", :require => false +gem "appraisal", "~> 2.2.0" +gem "rspec-its", require: false +gem "rspec-wait", require: false +gem "mock_redis" gem "sidekiq", "~> 4.0.0" platforms :mri_24 do - gem "benchmark-ips", :require => false - gem "codeclimate-test-reporter", :require => false - gem "fasterer", :require => false - gem "memory_profiler", :require => false - gem "mock_redis", :require => false - gem "pry-byebug", :require => false - gem "rubocop", :require => false - gem "simplecov-json", :require => false - gem "travis", :require => false + gem "benchmark-ips", require: false + gem "fasterer", require: false + gem "memory_profiler", require: false + gem "pry-byebug", require: false + gem "rubocop", require: false + gem "simplecov-json", require: false + gem "travis", require: false end -gemspec :path => "../" +gemspec path: "../" diff --git a/gemfiles/sidekiq_4.1.gemfile b/gemfiles/sidekiq_4.1.gemfile index ffceb7ed3..7a39c92a2 100644 --- a/gemfiles/sidekiq_4.1.gemfile +++ b/gemfiles/sidekiq_4.1.gemfile @@ -2,21 +2,20 @@ source "https://rubygems.org" -gem "appraisal", "~> 2.0.0" -gem "rspec-its", :require => false -gem "rspec-wait", :require => false +gem "appraisal", "~> 2.2.0" +gem "rspec-its", require: false +gem "rspec-wait", require: false +gem "mock_redis" gem "sidekiq", "~> 4.1.0" platforms :mri_24 do - gem "benchmark-ips", :require => false - gem "codeclimate-test-reporter", :require => false - gem "fasterer", :require => false - gem "memory_profiler", :require => false - gem "mock_redis", :require => false - gem "pry-byebug", :require => false - gem "rubocop", :require => false - gem "simplecov-json", :require => false - gem "travis", :require => false + gem "benchmark-ips", require: false + gem "fasterer", require: false + gem "memory_profiler", require: false + gem "pry-byebug", require: false + gem "rubocop", require: false + gem "simplecov-json", require: false + gem "travis", require: false end -gemspec :path => "../" +gemspec path: "../" diff --git a/gemfiles/sidekiq_4.2.gemfile b/gemfiles/sidekiq_4.2.gemfile index 7a787c4ab..a506c9eed 100644 --- a/gemfiles/sidekiq_4.2.gemfile +++ b/gemfiles/sidekiq_4.2.gemfile @@ -2,21 +2,20 @@ source "https://rubygems.org" -gem "appraisal", "~> 2.0.0" -gem "rspec-its", :require => false -gem "rspec-wait", :require => false +gem "appraisal", "~> 2.2.0" +gem "rspec-its", require: false +gem "rspec-wait", require: false +gem "mock_redis" gem "sidekiq", "~> 4.2.0" platforms :mri_24 do - gem "benchmark-ips", :require => false - gem "codeclimate-test-reporter", :require => false - gem "fasterer", :require => false - gem "memory_profiler", :require => false - gem "mock_redis", :require => false - gem "pry-byebug", :require => false - gem "rubocop", :require => false - gem "simplecov-json", :require => false - gem "travis", :require => false + gem "benchmark-ips", require: false + gem "fasterer", require: false + gem "memory_profiler", require: false + gem "pry-byebug", require: false + gem "rubocop", require: false + gem "simplecov-json", require: false + gem "travis", require: false end -gemspec :path => "../" +gemspec path: "../" diff --git a/gemfiles/sidekiq_5.0.gemfile b/gemfiles/sidekiq_5.0.gemfile index e3717c07e..2cff3d468 100644 --- a/gemfiles/sidekiq_5.0.gemfile +++ b/gemfiles/sidekiq_5.0.gemfile @@ -2,21 +2,20 @@ source "https://rubygems.org" -gem "appraisal", "~> 2.0.0" -gem "rspec-its", :require => false -gem "rspec-wait", :require => false +gem "appraisal", "~> 2.2.0" +gem "rspec-its", require: false +gem "rspec-wait", require: false +gem "mock_redis" gem "sidekiq", ">= 5.0.0.beta", "< 6" platforms :mri_24 do - gem "benchmark-ips", :require => false - gem "codeclimate-test-reporter", :require => false - gem "fasterer", :require => false - gem "memory_profiler", :require => false - gem "mock_redis", :require => false - gem "pry-byebug", :require => false - gem "rubocop", :require => false - gem "simplecov-json", :require => false - gem "travis", :require => false + gem "benchmark-ips", require: false + gem "fasterer", require: false + gem "memory_profiler", require: false + gem "pry-byebug", require: false + gem "rubocop", require: false + gem "simplecov-json", require: false + gem "travis", require: false end -gemspec :path => "../" +gemspec path: "../" diff --git a/gemfiles/sidekiq_develop.gemfile b/gemfiles/sidekiq_develop.gemfile index 3e3deafa9..87978fefe 100644 --- a/gemfiles/sidekiq_develop.gemfile +++ b/gemfiles/sidekiq_develop.gemfile @@ -2,21 +2,20 @@ source "https://rubygems.org" -gem "appraisal", "~> 2.0.0" -gem "rspec-its", :require => false -gem "rspec-wait", :require => false -gem "sidekiq", :github => "mperham/sidekiq" +gem "appraisal", "~> 2.2.0" +gem "rspec-its", require: false +gem "rspec-wait", require: false +gem "mock_redis" +gem "sidekiq", github: "mperham/sidekiq" platforms :mri_24 do - gem "benchmark-ips", :require => false - gem "codeclimate-test-reporter", :require => false - gem "fasterer", :require => false - gem "memory_profiler", :require => false - gem "mock_redis", :require => false - gem "pry-byebug", :require => false - gem "rubocop", :require => false - gem "simplecov-json", :require => false - gem "travis", :require => false + gem "benchmark-ips", require: false + gem "fasterer", require: false + gem "memory_profiler", require: false + gem "pry-byebug", require: false + gem "rubocop", require: false + gem "simplecov-json", require: false + gem "travis", require: false end -gemspec :path => "../" +gemspec path: "../" diff --git a/lib/sidekiq-unique-jobs.rb b/lib/sidekiq-unique-jobs.rb index e61026148..2d9ff0468 100644 --- a/lib/sidekiq-unique-jobs.rb +++ b/lib/sidekiq-unique-jobs.rb @@ -4,15 +4,16 @@ require 'forwardable' require 'sidekiq_unique_jobs/version' require 'sidekiq_unique_jobs/constants' +require 'sidekiq_unique_jobs/exceptions' require 'sidekiq_unique_jobs/util' require 'sidekiq_unique_jobs/cli' require 'sidekiq_unique_jobs/core_ext' -require 'sidekiq_unique_jobs/timeout_calculator' -require 'sidekiq_unique_jobs/options_with_fallback' +require 'sidekiq_unique_jobs/timeout' require 'sidekiq_unique_jobs/scripts' require 'sidekiq_unique_jobs/unique_args' require 'sidekiq_unique_jobs/unlockable' require 'sidekiq_unique_jobs/lock' +require 'sidekiq_unique_jobs/options_with_fallback' require 'sidekiq_unique_jobs/middleware' require 'sidekiq_unique_jobs/config' require 'sidekiq_unique_jobs/sidekiq_unique_ext' @@ -27,6 +28,7 @@ def config unique_prefix: 'uniquejobs', default_queue_lock_expiration: 30 * 60, default_run_lock_expiration: 60, + default_lock_timeout: 0, default_lock: :while_executing, redis_test_mode: :redis, # :mock raise_unique_args_errors: false, diff --git a/lib/sidekiq_unique_jobs/cli.rb b/lib/sidekiq_unique_jobs/cli.rb index 2223d8621..1c71489b3 100644 --- a/lib/sidekiq_unique_jobs/cli.rb +++ b/lib/sidekiq_unique_jobs/cli.rb @@ -28,13 +28,6 @@ def del(pattern) say "Deleted #{deleted_count} keys matching '#{pattern}'" end - desc 'expire', 'removes all expired unique keys from the hash in redis' - def expire - expired = Util.expire - say "Removed #{expired.values.size} left overs from redis." - print_in_columns(expired.values) - end - desc 'console', 'drop into a console with easy access to helper methods' def console say "Use `keys '*', 1000 to display the first 1000 unique keys matching '*'" diff --git a/lib/sidekiq_unique_jobs/client/middleware.rb b/lib/sidekiq_unique_jobs/client/middleware.rb index 52f4122a2..66b744f25 100644 --- a/lib/sidekiq_unique_jobs/client/middleware.rb +++ b/lib/sidekiq_unique_jobs/client/middleware.rb @@ -13,23 +13,22 @@ class Middleware def call(worker_class, item, queue, redis_pool = nil) @worker_class = worker_class_constantize(worker_class) + SidekiqUniqueJobs::UniqueArgs.digest(item) @item = item @queue = queue @redis_pool = redis_pool - - yield if disabled_or_successfully_locked? + yield if successfully_locked? end private attr_reader :item, :worker_class, :redis_pool, :queue - def disabled_or_successfully_locked? + def successfully_locked? unique_disabled? || acquire_lock end def acquire_lock - return true unless lock.respond_to?(:lock) locked = lock.lock(:client) warn_about_duplicate(item) unless locked locked diff --git a/lib/sidekiq_unique_jobs/constants.rb b/lib/sidekiq_unique_jobs/constants.rb index fb099f562..10cdca638 100644 --- a/lib/sidekiq_unique_jobs/constants.rb +++ b/lib/sidekiq_unique_jobs/constants.rb @@ -4,13 +4,17 @@ module SidekiqUniqueJobs ARGS_KEY ||= 'args' AT_KEY ||= 'at' CLASS_KEY ||= 'class' + LOCK_EXPIRATION_KEY ||= 'lock_expiration' JID_KEY ||= 'jid' + LOCK_TIMEOUT_KEY ||= 'lock_timeout' LOG_DUPLICATE_KEY ||= 'log_duplicate_payload' - QUEUE_KEY ||= 'queue' - HASH_KEY ||= 'uniquejobs' - QUEUE_LOCK_TIMEOUT_KEY ||= 'unique_expiration' - RUN_LOCK_TIMEOUT_KEY ||= 'run_lock_expiration' + LOCK_RESOURCES_KEY ||= 'lock_resources' + RESCHEDULE_KEY ||= 'reschedule' + RUN_LOCK_EXPIRATION_KEY ||= 'run_lock_expiration' + STALE_CLIENT_TIMEOUT_KEY ||= 'stale_client_timeout' TESTING_CONSTANT ||= 'Testing' + QUEUE_KEY ||= 'queue' + QUEUE_LOCK_EXPIRATION_KEY ||= 'unique_expiration' UNIQUE_KEY ||= 'unique' UNIQUE_LOCK_KEY ||= 'unique_lock' UNIQUE_ARGS_KEY ||= 'unique_args' @@ -19,4 +23,5 @@ module SidekiqUniqueJobs UNIQUE_ON_ALL_QUEUES_KEY ||= 'unique_on_all_queues' UNIQUE_ACROSS_WORKERS_KEY ||= 'unique_across_workers' UNIQUE_ARGS_ENABLED_KEY ||= 'unique_args_enabled' + USE_LOCAL_TIME_KEY ||= 'use_local_time' end diff --git a/lib/sidekiq_unique_jobs/exceptions.rb b/lib/sidekiq_unique_jobs/exceptions.rb new file mode 100644 index 000000000..e3fd9365d --- /dev/null +++ b/lib/sidekiq_unique_jobs/exceptions.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module SidekiqUniqueJobs + class LockTimeout < StandardError + end + + class RunLockFailed < StandardError + end + + class ScriptError < StandardError + end + + class UniqueKeyMissing < ArgumentError + end + + class JidMissing < ArgumentError + end + + class MaxLockTimeMissing < ArgumentError + end + + class UnexpectedValue < StandardError + end +end diff --git a/lib/sidekiq_unique_jobs/lock.rb b/lib/sidekiq_unique_jobs/lock.rb index 2566d58b2..ae94001e6 100644 --- a/lib/sidekiq_unique_jobs/lock.rb +++ b/lib/sidekiq_unique_jobs/lock.rb @@ -1,12 +1,232 @@ # frozen_string_literal: true +module SidekiqUniqueJobs + class Lock # rubocop:disable ClassLength + API_VERSION = '1' + EXPIRES_IN = 10 + + # stale_client_timeout is the threshold of time before we assume + # that something has gone terribly wrong with a client and we + # invalidate it's lock. + # Default is nil for which we don't check for stale clients + # SidekiqUniqueJobs::Lock::WhileExecuting.new(item, :stale_client_timeout => 30, :redis => myRedis) + # SidekiqUniqueJobs::Lock::WhileExecuting.new(item, :redis => myRedis) + # SidekiqUniqueJobs::Lock::WhileExecuting.new(item, :resources => 1, :redis => myRedis) + # SidekiqUniqueJobs::Lock::WhileExecuting.new(item, :host => "", :port => "") + # SidekiqUniqueJobs::Lock::WhileExecuting.new(item, :path => "bla") + def initialize(item, redis_pool = nil) + @item = item + @current_jid = @item[JID_KEY] + @unique_digest = @item[UNIQUE_DIGEST_KEY] + @redis_pool = redis_pool + @lock_expiration = @item[SidekiqUniqueJobs::LOCK_EXPIRATION_KEY] + @lock_timeout = @item[SidekiqUniqueJobs::LOCK_TIMEOUT_KEY] + @stale_client_timeout = @item[SidekiqUniqueJobs::STALE_CLIENT_TIMEOUT_KEY] + @use_local_time = @item[SidekiqUniqueJobs::USE_LOCAL_TIME_KEY] + @reschedule = @item[SidekiqUniqueJobs::RESCHEDULE_KEY] + @tokens = [] + end + + def exists_or_create! + SidekiqUniqueJobs::Scripts.call( + :exists_or_create, + @redis_pool, + keys: [exists_key, grabbed_key, available_key], + argv: [@current_jid, @lock_expiration], + ) + end + + def exists? + SidekiqUniqueJobs.connection(@redis_pool) do |conn| + conn.exists(exists_key) + end + end + + def available_count + SidekiqUniqueJobs.connection(@redis_pool) do |conn| + conn.llen(available_key) if conn.exists(exists_key) + end + end + + def delete! + SidekiqUniqueJobs.connection(@redis_pool) do |conn| + conn.del(available_key) + conn.del(grabbed_key) + conn.del(exists_key) + end + end + + def lock(timeout = nil) # rubocop:disable MethodLength + exists_or_create! + release_stale_locks! + + SidekiqUniqueJobs.connection(@redis_pool) do |conn| + if timeout.nil? || timeout.positive? + # passing timeout 0 to blpop causes it to block + _key, current_token = conn.blpop(available_key, timeout || 0) + else + current_token = conn.lpop(available_key) + end + + return false unless current_token == @current_jid + + conn.hset(grabbed_key, current_token, current_time.to_f) + return_value = current_token + + if block_given? + begin + return_value = yield current_token + ensure + signal(conn, current_token) + end + end + + return_value + end + end + alias wait lock + + def unlock + return false unless locked? + SidekiqUniqueJobs.connection(@redis_pool) do |conn| + signal(conn)[1] + end + + locked? + end + + def locked? + SidekiqUniqueJobs.connection(@redis_pool) do |conn| + conn.hexists(grabbed_key, @current_jid) + end + end + + def signal(conn, token = nil) + token ||= @current_jid + conn.multi do + conn.hdel grabbed_key, token + conn.lpush available_key, token + + expire_when_necessary(conn) + end + end + + def release_stale_locks! + return unless check_staleness? + + if SidekiqUniqueJobs.redis_version >= '3.2' + release_stale_locks_lua! + else + release_stale_locks_ruby! + end + end + + def available_key + @available_key ||= namespaced_key('AVAILABLE') + end + + def exists_key + @exists_key ||= namespaced_key('EXISTS') + end + + def grabbed_key + @grabbed_key ||= namespaced_key('GRABBED') + end + + def release_key + @release_key ||= namespaced_key('RELEASE') + end + + private + + def release_stale_locks_lua! + SidekiqUniqueJobs::Scripts.call( + :release_stale_locks, + @redis_pool, + keys: [exists_key, grabbed_key, available_key, release_key], + argv: [EXPIRES_IN, @stale_client_timeout, @lock_expiration], + ) + end + + def release_stale_locks_ruby! + SidekiqUniqueJobs.connection(@redis_pool) do |conn| + simple_expiring_mutex(conn) do + conn.hgetall(grabbed_key).each do |token, locked_at| + timed_out_at = locked_at.to_f + @stale_client_timeout + + signal(conn, token) if timed_out_at < current_time.to_f + end + end + end + end + + def simple_expiring_mutex(conn) + # Using the locking mechanism as described in + # http://redis.io/commands/setnx + + cached_current_time = current_time.to_f + my_lock_expires_at = cached_current_time + EXPIRES_IN + 1 + return false unless create_mutex(conn, my_lock_expires_at, cached_current_time) + + yield + ensure + # Make sure not to delete the lock in case someone else already expired + # our lock, with one second in between to account for some lag. + conn.del(release_key) if my_lock_expires_at > (current_time.to_f - 1) + end + + def create_mutex(conn, my_lock_expires_at, cached_current_time) + # return true if we got the lock + return true if conn.setnx(release_key, my_lock_expires_at) + + # Check if expired + other_lock_expires_at = conn.get(release_key).to_f + + return false unless other_lock_expires_at < cached_current_time + + old_expires_at = conn.getset(release_key, my_lock_expires_at).to_f + # Check if another client started cleanup yet. If not, + # then we now have the lock. + old_expires_at == other_lock_expires_at + end + + def expire_when_necessary(conn) + return if @lock_expiration.nil? + + [available_key, exists_key].each do |key| + conn.expire(key, @lock_expiration) + end + end + + def check_staleness? + !@stale_client_timeout.nil? + end + + def namespaced_key(variable) + "#{@unique_digest}:#{variable}" + end + + def current_time + if @use_local_time + Time.now + else + begin + instant = SidekiqUniqueJobs.connection(@redis_pool, &:time) + Time.at(instant[0], instant[1]) + rescue + @use_local_time = true + current_time + end + end + end + end +end + +require 'sidekiq_unique_jobs/lock/prepares_items' +require 'sidekiq_unique_jobs/lock/queue_lock_base' +require 'sidekiq_unique_jobs/lock/run_lock_base' require 'sidekiq_unique_jobs/lock/until_executed' require 'sidekiq_unique_jobs/lock/until_executing' require 'sidekiq_unique_jobs/lock/while_executing' require 'sidekiq_unique_jobs/lock/until_timeout' require 'sidekiq_unique_jobs/lock/until_and_while_executing' - -module SidekiqUniqueJobs - module Lock - end -end diff --git a/lib/sidekiq_unique_jobs/lock/prepares_items.rb b/lib/sidekiq_unique_jobs/lock/prepares_items.rb new file mode 100644 index 000000000..74126ca39 --- /dev/null +++ b/lib/sidekiq_unique_jobs/lock/prepares_items.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +module SidekiqUniqueJobs + class Lock + module PreparesItems + def prepare_item(item) + item[LOCK_TIMEOUT_KEY] = @calculator.lock_timeout + item[LOCK_EXPIRATION_KEY] = @calculator.lock_expiration + SidekiqUniqueJobs::UniqueArgs.digest(item) + item + end + end + end +end diff --git a/lib/sidekiq_unique_jobs/lock/queue_lock_base.rb b/lib/sidekiq_unique_jobs/lock/queue_lock_base.rb new file mode 100644 index 000000000..a26ef85d4 --- /dev/null +++ b/lib/sidekiq_unique_jobs/lock/queue_lock_base.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +module SidekiqUniqueJobs + class Lock + class QueueLockBase + include SidekiqUniqueJobs::Lock::PreparesItems + + def initialize(item, redis_pool = nil) + @calculator = SidekiqUniqueJobs::Timeout::QueueLock.new(item) + @item = prepare_item(item) + @redis_pool = redis_pool + @lock = SidekiqUniqueJobs::Lock.new(@item, @redis_pool) + end + + def lock(_scope) + raise NotImplementedError, "##{__method__} needs to be implemented in #{self.class}" + end + + def execute(_callback = nil) + raise NotImplementedError, "##{__method__} needs to be implemented in #{self.class}" + end + + def unlock(_scope) + raise NotImplementedError, "##{__method__} needs to be implemented in #{self.class}" + end + + private + + def validate_scope!(actual_scope:, expected_scope:) + return if actual_scope == expected_scope + raise ArgumentError, "`#{actual_scope}` client middleware can't unlock #{@item[UNIQUE_DIGEST_KEY]}" + end + end + end +end diff --git a/lib/sidekiq_unique_jobs/lock/reschedule_while_executing.rb b/lib/sidekiq_unique_jobs/lock/reschedule_while_executing.rb new file mode 100644 index 000000000..2a6eff08b --- /dev/null +++ b/lib/sidekiq_unique_jobs/lock/reschedule_while_executing.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module SidekiqUniqueJobs + class Lock + class RescheduleWhileExecuting < RunLockBase + def lock(_scope) + true + end + + def execute(callback = nil) + @lock.lock do + yield + callback.call + end + + Sidekiq::Client.push(@item) unless @lock.locked? + end + + def unlock + @lock.unlock('0') + end + end + end +end diff --git a/lib/sidekiq_unique_jobs/lock/run_lock_base.rb b/lib/sidekiq_unique_jobs/lock/run_lock_base.rb new file mode 100644 index 000000000..f9ead5fc5 --- /dev/null +++ b/lib/sidekiq_unique_jobs/lock/run_lock_base.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +module SidekiqUniqueJobs + class Lock + class RunLockBase + include SidekiqUniqueJobs::Lock::PreparesItems + + def initialize(item, redis_pool = nil) + @calculator = Timeout::RunLock.new(item) + @item = prepare_item(item) + @redis_pool = redis_pool + @lock = SidekiqUniqueJobs::Lock.new(@item) + end + + def lock(_scope) + raise NotImplementedError, "##{__method__} needs to be implemented in #{self.class}" + end + + def execute(_callback = nil) + raise NotImplementedError, "##{__method__} needs to be implemented in #{self.class}" + end + + def unlock(_scope) + raise NotImplementedError, "##{__method__} needs to be implemented in #{self.class}" + end + + private + + def fail_with_lock_timeout! + return if @calculator.lock_timeout.to_i <= 0 + + raise(SidekiqUniqueJobs::LockTimeout, + "couldn't achieve lock for #{@lock.available_key} within: #{@calculator.lock_timeout} seconds") + end + end + 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 1280dfbf5..d5eca649f 100644 --- a/lib/sidekiq_unique_jobs/lock/until_and_while_executing.rb +++ b/lib/sidekiq_unique_jobs/lock/until_and_while_executing.rb @@ -1,15 +1,41 @@ # frozen_string_literal: true module SidekiqUniqueJobs - module Lock - class UntilAndWhileExecuting < UntilExecuting + class Lock + class UntilAndWhileExecuting < QueueLockBase + # Lock item until the server processing starts + # + # @param scope [Symbol] the scope, `:client` or `:server` + # @return [Boolean] truthy for success + # @raise [ArgumentError] if scope != :client + def lock(scope) + validate_scope!(actual_scope: scope, expected_scope: :client) + + @lock.lock(0) + end + + # Unlocks the current job, then lock for processing + # + # @param callback [Proc] callback to call when finished + # @return [Boolean] report success + # @raise [SidekiqUniqueJobs::LockTimeout] when lock fails within configured timeout def execute(callback) - lock = WhileExecuting.new(item, redis_pool) - lock.synchronize do - callback.call if unlock(:server) + unlock(:server) + + runtime_lock.execute(callback) do yield end end + + def unlock(scope) + validate_scope!(actual_scope: scope, expected_scope: :server) + + @lock.unlock + end + + def runtime_lock + @runtime_lock ||= SidekiqUniqueJobs::Lock::WhileExecuting.new(@item, @redis_pool) + end end end end diff --git a/lib/sidekiq_unique_jobs/lock/until_executed.rb b/lib/sidekiq_unique_jobs/lock/until_executed.rb index 0ff58fb1e..d1a9b1132 100644 --- a/lib/sidekiq_unique_jobs/lock/until_executed.rb +++ b/lib/sidekiq_unique_jobs/lock/until_executed.rb @@ -1,23 +1,16 @@ # frozen_string_literal: true module SidekiqUniqueJobs - module Lock - class UntilExecuted + class Lock + class UntilExecuted < QueueLockBase OK ||= 'OK' - include SidekiqUniqueJobs::Unlockable - extend Forwardable def_delegators :Sidekiq, :logger - def initialize(item, redis_pool = nil) - @item = item - @redis_pool = redis_pool - end - - def execute(callback, &blk) + def execute(callback, &block) operative = true - send(:after_yield_yield, &blk) + send(:after_yield_yield, &block) rescue Sidekiq::Shutdown operative = false raise @@ -25,47 +18,25 @@ def execute(callback, &blk) if operative && unlock(:server) callback.call else - logger.fatal { "the unique_key: #{unique_key} needs to be unlocked manually" } + logger.fatal("the unique_key: #{@item[UNIQUE_DIGEST_KEY]} needs to be unlocked manually") end end def unlock(scope) - unless [:server, :api, :test].include?(scope) - raise ArgumentError, "#{scope} middleware can't #{__method__} #{unique_key}" - end + validate_scope!(actual_scope: scope, expected_scope: :server) - unlock_by_key(unique_key, item[JID_KEY], redis_pool) + @lock.unlock + @lock.delete! end def lock(scope) - if scope.to_sym != :client - raise ArgumentError, "#{scope} middleware can't #{__method__} #{unique_key}" - end - - Scripts::AcquireLock.execute( - redis_pool, - unique_key, - item[JID_KEY], - max_lock_time, - ) - end - # rubocop:enable MethodLength - - def unique_key - @unique_key ||= UniqueArgs.digest(item) - end - - def max_lock_time - @max_lock_time ||= QueueLockTimeoutCalculator.for_item(item).seconds + validate_scope!(actual_scope: scope, expected_scope: :client) + @lock.lock(@calculator.lock_timeout) end def after_yield_yield yield end - - private - - attr_reader :item, :redis_pool end end end diff --git a/lib/sidekiq_unique_jobs/lock/until_executing.rb b/lib/sidekiq_unique_jobs/lock/until_executing.rb index efd23e3ea..e40c37463 100644 --- a/lib/sidekiq_unique_jobs/lock/until_executing.rb +++ b/lib/sidekiq_unique_jobs/lock/until_executing.rb @@ -1,9 +1,38 @@ # frozen_string_literal: true module SidekiqUniqueJobs - module Lock - class UntilExecuting < UntilExecuted - def execute(callback, &_block) + class Lock + class UntilExecuting < QueueLockBase + # Lock when client/middleware is running + # + # @param scope [Symbol] the scope, `:client` or `:server` + # @return [Boolean] report success + # @raise [ArgumentError] if scope != :client + def lock(scope) + validate_scope!(actual_scope: scope, expected_scope: :client) + + @lock.lock(0) + end + + # Lock when server/middleware is running + # + # @param scope [Symbol] the scope, `:client` or `:server` + # @return [Boolean] report success + # @raise [ArgumentError] if scope != :server + def unlock(scope) + validate_scope!(actual_scope: scope, expected_scope: :server) + + @lock.unlock + end + + # Executes the job + # before it gets executed we unlock it to enable other jobs with + # the same arguments to be scheduled. + # + # @param callback [Proc] callback to call when finished + # @return [Boolean] report success + # @raise [SidekiqUniqueJobs::LockTimeout] if we timed out when acquiring lock + def execute(callback) callback.call if unlock(:server) yield end diff --git a/lib/sidekiq_unique_jobs/lock/until_timeout.rb b/lib/sidekiq_unique_jobs/lock/until_timeout.rb index 22ef2ea8c..ca9df3bc6 100644 --- a/lib/sidekiq_unique_jobs/lock/until_timeout.rb +++ b/lib/sidekiq_unique_jobs/lock/until_timeout.rb @@ -1,14 +1,32 @@ # frozen_string_literal: true module SidekiqUniqueJobs - module Lock - class UntilTimeout < UntilExecuted - def unlock(scope) - return true if scope.to_sym == :server + class Lock + class UntilTimeout < QueueLockBase + # Lock item until it expires + # + # @param scope [Symbol] the scope, `:client` or `:server` + # @return [Boolean] truthy for success + # @raise [ArgumentError] if scope != :client + def lock(scope) + validate_scope!(actual_scope: scope, expected_scope: :client) + + @lock.lock(0) + end - raise ArgumentError, "#{scope} middleware can't #{__method__} #{unique_key}" + # Lock item until it expires + # + # @param scope [Symbol] the scope, `:client` or `:server` + # @return [Boolean] returns true + # @raise [ArgumentError] if scope != :server + def unlock(scope) + validate_scope!(actual_scope: scope, expected_scope: :server) + true end + # Execute the block + # + # @param _callback [Proc] callback that will never get called def execute(_callback) yield if block_given? end diff --git a/lib/sidekiq_unique_jobs/lock/while_executing.rb b/lib/sidekiq_unique_jobs/lock/while_executing.rb index 0e25733b4..4d492a08e 100644 --- a/lib/sidekiq_unique_jobs/lock/while_executing.rb +++ b/lib/sidekiq_unique_jobs/lock/while_executing.rb @@ -1,50 +1,37 @@ # frozen_string_literal: true module SidekiqUniqueJobs - module Lock - class WhileExecuting - def self.synchronize(item, redis_pool = nil) - new(item, redis_pool).synchronize { yield } + class Lock + class WhileExecuting < RunLockBase + # Don't lock when client middleware runs + # + # @param _scope [Symbol] the scope, `:client` or `:server` + # @return [Boolean] always returns true + def lock(_scope) + true end - def initialize(item, redis_pool = nil) - @item = item - @redis_pool = redis_pool - @unique_digest = "#{create_digest}:run" - @mutex = Mutex.new - end - - def synchronize - @mutex.synchronize do - sleep 0.1 until locked? - yield - end - rescue Sidekiq::Shutdown - logger.fatal { "the unique_key: #{@unique_digest} needs to be unlocked manually" } - raise - ensure - SidekiqUniqueJobs.connection(@redis_pool) { |conn| conn.del @unique_digest } - end - - def locked? - Scripts.call(:synchronize, @redis_pool, - keys: [@unique_digest], - argv: [Time.now.to_i, max_lock_time]) == 1 - end - - def max_lock_time - @max_lock_time ||= RunLockTimeoutCalculator.for_item(@item).seconds - end - - def execute(_callback) - synchronize do + # Locks while server middleware executes the job + # + # @param callback [Proc] callback to call when finished + # @return [Boolean] report success + # @raise [SidekiqUniqueJobs::LockTimeout] when lock fails within configured timeout + def execute(callback) + jid = @lock.lock + if jid == @item[JID_KEY] + callback&.call + unlock(:server) yield + else + fail_with_lock_timeout! end end - def create_digest - @unique_digest ||= @item[UNIQUE_DIGEST_KEY] - @unique_digest ||= SidekiqUniqueJobs::UniqueArgs.digest(@item) + # Unlock the current item + # + def unlock(_scope) + @lock.unlock + @lock.delete! end end end diff --git a/lib/sidekiq_unique_jobs/options_with_fallback.rb b/lib/sidekiq_unique_jobs/options_with_fallback.rb index 981e5bba3..fa5dfcaf8 100644 --- a/lib/sidekiq_unique_jobs/options_with_fallback.rb +++ b/lib/sidekiq_unique_jobs/options_with_fallback.rb @@ -2,9 +2,13 @@ module SidekiqUniqueJobs module OptionsWithFallback - def self.included(base) - base.send(:extend, SidekiqUniqueJobs::OptionsWithFallback::ClassMethods) - end + LOCKS = { + until_and_while_executing: SidekiqUniqueJobs::Lock::UntilAndWhileExecuting, + until_executed: SidekiqUniqueJobs::Lock::UntilExecuted, + until_executing: SidekiqUniqueJobs::Lock::UntilExecuting, + until_timeout: SidekiqUniqueJobs::Lock::UntilTimeout, + while_executing: SidekiqUniqueJobs::Lock::WhileExecuting, + }.freeze def unique_enabled? options[UNIQUE_KEY] || item[UNIQUE_KEY] @@ -23,7 +27,7 @@ def lock end def lock_class - lock_cache[unique_lock.to_sym] ||= Object.const_get("SidekiqUniqueJobs::Lock::#{unique_lock.to_s.classify}") + @lock_class ||= LOCKS[unique_lock.to_sym] end def unique_lock @@ -46,29 +50,11 @@ def lock_type_from(hash, key = UNIQUE_KEY) hash[key] end - def lock_cache - self.class.lock_cache - end - - def lock_cache=(obj) - self.class.lock_cache = obj - end - def options @options ||= worker_class.get_sidekiq_options if worker_class.respond_to?(:get_sidekiq_options) @options ||= Sidekiq.default_worker_options @options ||= {} @options &&= @options.stringify_keys end - - module ClassMethods - def lock_cache - @lock_cache ||= {} - end - - def lock_cache=(obj) - @lock_cache = obj - end - end end end diff --git a/lib/sidekiq_unique_jobs/run_lock_failed.rb b/lib/sidekiq_unique_jobs/run_lock_failed.rb deleted file mode 100644 index b62f6a8d1..000000000 --- a/lib/sidekiq_unique_jobs/run_lock_failed.rb +++ /dev/null @@ -1,3 +0,0 @@ -# frozen_string_literal: true - -SidekiqUniqueJobs::RunLockFailed = Class.new(StandardError) diff --git a/lib/sidekiq_unique_jobs/script_mock.rb b/lib/sidekiq_unique_jobs/script_mock.rb deleted file mode 100644 index 540f7c77b..000000000 --- a/lib/sidekiq_unique_jobs/script_mock.rb +++ /dev/null @@ -1,69 +0,0 @@ -# frozen_string_literal: true - -require 'pathname' -require 'digest/sha1' - -module SidekiqUniqueJobs - module ScriptMock - module_function - - extend SingleForwardable - def_delegator :SidekiqUniqueJobs, :connection - - def call(file_name, redis_pool, options = {}) - send(file_name, redis_pool, options) - end - - def acquire_lock(redis_pool, options = {}) - connection(redis_pool) do |conn| - unique_key = options[:keys][0] - job_id = options[:argv][0] - expires = options[:argv][1].to_i - stored_jid = conn.get(unique_key) - - return (stored_jid == job_id) ? 1 : 0 if stored_jid - return 0 unless conn.set(unique_key, job_id, nx: true, ex: expires) - - conn.hsetnx(SidekiqUniqueJobs::HASH_KEY, job_id, unique_key) - - return 1 - end - end - - def release_lock(redis_pool, options = {}) - connection(redis_pool) do |conn| - unique_key = options[:keys][0] - job_id = options[:argv][0] - stored_jid = conn.get(unique_key) - - return -1 unless stored_jid - return 0 unless stored_jid == job_id || stored_jid == '2' - - conn.del(unique_key) - conn.hdel(SidekiqUniqueJobs::HASH_KEY, job_id) - - return 1 - end - end - - def synchronize(redis_pool, options = {}) - connection(redis_pool) do |conn| - unique_key = options[:keys][0] - time = options[:argv][0].to_i - expires = options[:argv][1].to_f - - return 1 if conn.set(unique_key, time + expires, nx: true, ex: expires) - - stored_time = conn.get(unique_key) - if stored_time && stored_time < time - if conn.set(unique_key, time + expires, xx: true, ex: expires) - return 1 - end - end - - return 0 - end - end - end - # rubocop:enable MethodLength -end diff --git a/lib/sidekiq_unique_jobs/scripts.rb b/lib/sidekiq_unique_jobs/scripts.rb index 475919176..7dad3383e 100644 --- a/lib/sidekiq_unique_jobs/scripts.rb +++ b/lib/sidekiq_unique_jobs/scripts.rb @@ -3,16 +3,8 @@ require 'pathname' require 'digest/sha1' require 'concurrent/map' -require 'sidekiq_unique_jobs/scripts/acquire_lock' -require 'sidekiq_unique_jobs/scripts/release_lock' module SidekiqUniqueJobs - ScriptError = Class.new(StandardError) - UniqueKeyMissing = Class.new(ArgumentError) - JidMissing = Class.new(ArgumentError) - MaxLockTimeMissing = Class.new(ArgumentError) - UnexpectedValue = Class.new(StandardError) - module Scripts LUA_PATHNAME ||= Pathname.new(__FILE__).dirname.join('../../redis').freeze SOURCE_FILES ||= Dir[LUA_PATHNAME.join('**/*.lua')].compact.freeze @@ -42,7 +34,7 @@ def handle_error(ex, file_name, redis_pool, options = {}) SCRIPT_SHAS.delete(file_name) call(file_name, redis_pool, options) else - raise ScriptError, "Problem compiling #{file_name}. Invalid LUA syntax?" + raise ScriptError, "Problem compiling #{file_name}. Message: #{ex.message}" end end diff --git a/lib/sidekiq_unique_jobs/scripts/acquire_lock.rb b/lib/sidekiq_unique_jobs/scripts/acquire_lock.rb deleted file mode 100644 index 2ab340daf..000000000 --- a/lib/sidekiq_unique_jobs/scripts/acquire_lock.rb +++ /dev/null @@ -1,47 +0,0 @@ -# frozen_string_literal: true - -module SidekiqUniqueJobs - module Scripts - class AcquireLock - extend Forwardable - def_delegator SidekiqUniqueJobs, :logger - - def self.execute(redis_pool, unique_key, jid, max_lock_time) - new(redis_pool, unique_key, jid, max_lock_time).execute - end - - attr_reader :redis_pool, :unique_key, :jid, :max_lock_time - - def initialize(_redis_pool, unique_key, jid, max_lock_time) - raise UniqueKeyMissing, 'unique_key is required' if unique_key.nil? - raise JidMissing, 'jid is required' if jid.nil? - raise MaxLockTimeMissing, 'max_lock_time is required' if max_lock_time.nil? - - @unique_key = unique_key - @jid = jid - @max_lock_time = max_lock_time - end - - def execute - result = Scripts.call(:acquire_lock, redis_pool, - keys: [unique_key], - argv: [jid, max_lock_time]) - - handle_result(result) - end - - def handle_result(result) - case result - when 1 - logger.debug { "successfully acquired lock #{unique_key} for #{max_lock_time} seconds" } - true - when 0 - logger.debug { "failed to acquire lock for #{unique_key}" } - false - else - raise UnexpectedValue, "failed to acquire lock : unexpected return value (#{result})" - end - end - end - end -end diff --git a/lib/sidekiq_unique_jobs/scripts/release_lock.rb b/lib/sidekiq_unique_jobs/scripts/release_lock.rb deleted file mode 100644 index d91519020..000000000 --- a/lib/sidekiq_unique_jobs/scripts/release_lock.rb +++ /dev/null @@ -1,49 +0,0 @@ -# frozen_string_literal: true - -module SidekiqUniqueJobs - module Scripts - class ReleaseLock - extend Forwardable - def_delegator SidekiqUniqueJobs, :logger - - def self.execute(redis_pool, unique_key, jid) - new(redis_pool, unique_key, jid).execute - end - - attr_reader :redis_pool, :unique_key, :jid - - def initialize(redis_pool, unique_key, jid) - raise UniqueKeyMissing, 'unique_key is required' if unique_key.nil? - raise JidMissing, 'jid is required' if jid.nil? - - @redis_pool = redis_pool - @unique_key = unique_key - @jid = jid - end - - def execute - result = Scripts.call(:release_lock, redis_pool, - keys: [unique_key], - argv: [jid]) - - handle_result(result) - end - - def handle_result(result) - case result - when 1 - logger.debug { "successfully unlocked #{unique_key}" } - true - when 0 - logger.debug { "expiring lock #{unique_key} is not owned by #{jid}" } - false - when -1 - logger.debug { "#{unique_key} is not a known key" } - false - else - raise UnexpectedValue, "failed to release lock : unexpected return value (#{result})" - end - end - end - end -end diff --git a/lib/sidekiq_unique_jobs/server/middleware.rb b/lib/sidekiq_unique_jobs/server/middleware.rb index 8eac7cb6d..124adcdd1 100644 --- a/lib/sidekiq_unique_jobs/server/middleware.rb +++ b/lib/sidekiq_unique_jobs/server/middleware.rb @@ -9,13 +9,17 @@ class Middleware include OptionsWithFallback - def call(worker, item, queue, redis_pool = nil, &blk) - @worker = worker + def call(worker, item, queue, redis_pool = nil) + SidekiqUniqueJobs::UniqueArgs.digest(item) + @worker = worker @redis_pool = redis_pool - @queue = queue - @item = item - return yield unless unique_enabled? - lock.send(:execute, after_unlock_hook, &blk) + @queue = queue + @item = item + return yield if unique_disabled? + + lock.execute(after_unlock_hook) do + yield + end end private diff --git a/lib/sidekiq_unique_jobs/sidekiq_unique_ext.rb b/lib/sidekiq_unique_jobs/sidekiq_unique_ext.rb index 323df45e5..cdc540524 100644 --- a/lib/sidekiq_unique_jobs/sidekiq_unique_ext.rb +++ b/lib/sidekiq_unique_jobs/sidekiq_unique_ext.rb @@ -7,7 +7,6 @@ class SortedEntry module UniqueExtension def self.included(base) base.class_eval do - include SidekiqUniqueJobs::Unlockable alias_method :delete_orig, :delete alias_method :delete, :delete_ext alias_method :remove_job_orig, :remove_job @@ -16,14 +15,14 @@ def self.included(base) end def delete_ext - SidekiqUniqueJobs::Unlockable.unlock(item) if delete_orig + SidekiqUniqueJobs::Unlockable.delete!(item) if delete_orig end private def remove_job_ext remove_job_orig do |message| - SidekiqUniqueJobs::Unlockable.unlock(Sidekiq.load_json(message)) + SidekiqUniqueJobs::Unlockable.delete!(Sidekiq.load_json(message)) yield message end end @@ -36,21 +35,13 @@ class ScheduledSet module UniqueExtension def self.included(base) base.class_eval do - include SidekiqUniqueJobs::Unlockable alias_method :delete_orig, :delete alias_method :delete, :delete_ext end end def delete_ext - SidekiqUniqueJobs::Unlockable.unlock(item) if delete_orig - end - - def remove_job_ext - remove_job_orig do |message| - SidekiqUniqueJobs::Unlockable.unlock(Sidekiq.load_json(message)) - yield message - end + SidekiqUniqueJobs::Unlockable.delete!(item) if delete_orig end end include UniqueExtension @@ -60,15 +51,14 @@ class Job module UniqueExtension def self.included(base) base.class_eval do - include SidekiqUniqueJobs::Unlockable alias_method :delete_orig, :delete alias_method :delete, :delete_ext end end def delete_ext - SidekiqUniqueJobs::Unlockable.unlock(item) delete_orig + SidekiqUniqueJobs::Unlockable.delete!(item) end end @@ -79,7 +69,6 @@ class Queue module UniqueExtension def self.included(base) base.class_eval do - include SidekiqUniqueJobs::Unlockable alias_method :clear_orig, :clear alias_method :clear, :clear_ext end @@ -98,7 +87,6 @@ class JobSet module UniqueExtension def self.included(base) base.class_eval do - include SidekiqUniqueJobs::Unlockable if base.method_defined?(:clear) alias_method :clear_orig, :clear alias_method :clear, :clear_ext @@ -117,7 +105,7 @@ def clear_ext end def delete_by_value_ext(name, value) - SidekiqUniqueJobs::Unlockable.unlock(Sidekiq.load_json(value)) if delete_by_value_orig(name, value) + SidekiqUniqueJobs::Unlockable.delete!(Sidekiq.load_json(value)) if delete_by_value_orig(name, value) end end diff --git a/lib/sidekiq_unique_jobs/testing.rb b/lib/sidekiq_unique_jobs/testing.rb index c917a5087..262d135b9 100644 --- a/lib/sidekiq_unique_jobs/testing.rb +++ b/lib/sidekiq_unique_jobs/testing.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true require 'sidekiq_unique_jobs/testing/sidekiq_overrides' -require 'sidekiq_unique_jobs/script_mock' module SidekiqUniqueJobs alias redis_version_real redis_version @@ -13,30 +12,81 @@ def redis_version end end - module Scripts - module Overrides + class Lock + module Testing def self.included(base) - base.extend Testing base.class_eval do - class << self - alias_method :call_orig, :call - alias_method :call, :call_ext + alias_method :exists_or_create_orig!, :exists_or_create! + alias_method :exists_or_create!, :exists_or_create_ext! + + alias_method :lock_orig, :lock + alias_method :lock, :lock_ext + end + end + + def exists_or_create_ext! # rubocop:disable Metrics/MethodLength + return exists_or_create_orig! unless SidekiqUniqueJobs.mocked? + + SidekiqUniqueJobs.connection do |conn| + current_token = conn.getset(exists_key, @current_jid) + + if current_token.nil? + conn.expire(exists_key, 10) + + conn.multi do + conn.del(grabbed_key) + conn.del(available_key) + conn.rpush(available_key, @current_jid) + conn.persist(exists_key) + + expire_when_necessary(conn) + end + + @current_jid + else + current_token end end end - module Testing - def call_ext(file_name, redis_pool, options = {}) - if SidekiqUniqueJobs.mocked? - SidekiqUniqueJobs::ScriptMock.call(file_name, redis_pool, options) + def lock_ext(timeout = nil) # rubocop:disable Metrics/LineLength, Metrics/MethodLength, Metrics/PerceivedComplexity, Metrics/CyclomaticComplexity + unless SidekiqUniqueJobs.mocked? + return lock_orig(timeout) unless block_given? + return lock_orig(timeout) do |_token| + yield + end + end + + SidekiqUniqueJobs.connection(@redis_pool) do |conn| + exists_or_create_ext! + release_stale_locks! + + if timeout.nil? || timeout.positive? + # passing timeout 0 to blpop causes it to block + _key, current_token = conn.blpop(available_key, timeout || 0) else - call_orig(file_name, redis_pool, options) + current_token = conn.lpop(available_key) end + + return false if current_token != @current_jid + + conn.hset(grabbed_key, current_token, current_time.to_f) + return_value = current_token + + if block_given? + begin + return_value = yield current_token + ensure + signal(conn, current_token) + end + end + + return_value end end end - include Overrides + include Testing end module Client @@ -44,10 +94,9 @@ class Middleware alias call_real call def call(worker_class, item, queue, redis_pool = nil) worker_class = SidekiqUniqueJobs.worker_class_constantize(worker_class) - if Sidekiq::Testing.inline? call_real(worker_class, item, queue, redis_pool) do - _server.call(worker_class.new, item, queue, redis_pool) do + server_middleware.call(worker_class.new, item, queue, redis_pool) do yield end end @@ -58,15 +107,9 @@ def call(worker_class, item, queue, redis_pool = nil) end end - def _server + def server_middleware SidekiqUniqueJobs::Server::Middleware.new end end end - - class Testing - def mocking! - require 'sidekiq_unique_jobs/testing/mocking' - end - end end diff --git a/lib/sidekiq_unique_jobs/testing/sidekiq_overrides.rb b/lib/sidekiq_unique_jobs/testing/sidekiq_overrides.rb index d85e6d186..baa9637bc 100644 --- a/lib/sidekiq_unique_jobs/testing/sidekiq_overrides.rb +++ b/lib/sidekiq_unique_jobs/testing/sidekiq_overrides.rb @@ -8,22 +8,12 @@ module ClassMethods # Clear all jobs for this worker def clear jobs.each do |job| - unlock(job) if Sidekiq::Testing.fake? + SidekiqUniqueJobs::Unlockable.delete!(job) end Sidekiq::Queues[queue].clear jobs.clear end - - unless respond_to?(:execute_job) - def execute_job(worker, args) - worker.perform(*args) - end - end - - def unlock(job) - SidekiqUniqueJobs::Unlockable.unlock(job) - end end module Overrides @@ -39,8 +29,8 @@ class << self module Testing def clear_all_ext - SidekiqUniqueJobs::Util.del('*', 1000, false) unless SidekiqUniqueJobs.mocked? clear_all_orig + SidekiqUniqueJobs::Util.del('*', 1000, false) end end end diff --git a/lib/sidekiq_unique_jobs/timeout.rb b/lib/sidekiq_unique_jobs/timeout.rb new file mode 100644 index 000000000..b021e5256 --- /dev/null +++ b/lib/sidekiq_unique_jobs/timeout.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +module SidekiqUniqueJobs + module Timeout + end +end + +require 'sidekiq_unique_jobs/timeout/calculator' +require 'sidekiq_unique_jobs/timeout/run_lock' +require 'sidekiq_unique_jobs/timeout/queue_lock' diff --git a/lib/sidekiq_unique_jobs/timeout/calculator.rb b/lib/sidekiq_unique_jobs/timeout/calculator.rb new file mode 100644 index 000000000..4bf0c3c08 --- /dev/null +++ b/lib/sidekiq_unique_jobs/timeout/calculator.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +module SidekiqUniqueJobs + module Timeout + class Calculator + def initialize(item) + @item = item + end + + def time_until_scheduled + return 0 unless @item[AT_KEY] + (Time.at(@item[AT_KEY]) - Time.now.utc).to_i + end + + def seconds + raise NotImplementedError, "#{__method__} needs to be implemented in #{self.class}" + end + + def lock_timeout + @lock_timeout ||= @item[LOCK_TIMEOUT_KEY] + @lock_timeout ||= worker_class_lock_timeout + @lock_timeout ||= SidekiqUniqueJobs.config.default_lock_timeout + end + + def worker_class_lock_timeout + worker_class_lock_expiration_for(LOCK_TIMEOUT_KEY) + end + + def worker_class_queue_lock_expiration + worker_class_lock_expiration_for(QUEUE_LOCK_EXPIRATION_KEY) + end + + def worker_class_run_lock_expiration + worker_class_lock_expiration_for(RUN_LOCK_EXPIRATION_KEY) + end + + def worker_class_lock_expiration + worker_class_lock_expiration_for(LOCK_EXPIRATION_KEY) + end + + def worker_class + @worker_class ||= SidekiqUniqueJobs.worker_class_constantize(@item[CLASS_KEY]) + end + + def worker_class_lock_expiration_for(key) + return unless worker_class.respond_to?(:get_sidekiq_options) + worker_class.get_sidekiq_options[key] + end + end + end +end diff --git a/lib/sidekiq_unique_jobs/timeout/queue_lock.rb b/lib/sidekiq_unique_jobs/timeout/queue_lock.rb new file mode 100644 index 000000000..4f71c2458 --- /dev/null +++ b/lib/sidekiq_unique_jobs/timeout/queue_lock.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module SidekiqUniqueJobs + module Timeout + class QueueLock < Timeout::Calculator + def lock_expiration + @lock_expiration ||= worker_class_lock_expiration + @lock_expiration ||= worker_class_queue_lock_expiration + @lock_expiration ||= SidekiqUniqueJobs.config.default_queue_lock_expiration + @lock_expiration = @item[LOCK_EXPIRATION_KEY] if @item.key?(LOCK_EXPIRATION_KEY) + @lock_expiration.to_i + time_until_scheduled if @lock_expiration + end + end + end +end diff --git a/lib/sidekiq_unique_jobs/timeout/run_lock.rb b/lib/sidekiq_unique_jobs/timeout/run_lock.rb new file mode 100644 index 000000000..4be9b1b4b --- /dev/null +++ b/lib/sidekiq_unique_jobs/timeout/run_lock.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +module SidekiqUniqueJobs + module Timeout + class RunLock < Timeout::Calculator + def lock_expiration + @lock_expiration ||= @item[LOCK_EXPIRATION_KEY] + @lock_expiration ||= worker_class_lock_expiration + @lock_expiration ||= worker_class_run_lock_expiration + @lock_expiration ||= SidekiqUniqueJobs.config.default_run_lock_expiration + end + end + end +end diff --git a/lib/sidekiq_unique_jobs/timeout_calculator.rb b/lib/sidekiq_unique_jobs/timeout_calculator.rb deleted file mode 100644 index edc608b00..000000000 --- a/lib/sidekiq_unique_jobs/timeout_calculator.rb +++ /dev/null @@ -1,67 +0,0 @@ -# frozen_string_literal: true - -module SidekiqUniqueJobs - class TimeoutCalculator - def self.for_item(item) - new(item) - end - - def initialize(item) - @item = item - end - - def time_until_scheduled - scheduled = item[AT_KEY] - return 0 unless scheduled - (Time.at(scheduled) - Time.now.utc).to_i - end - - def seconds - raise NotImplementedError - end - - def worker_class_queue_lock_expiration - worker_class_expiration_for QUEUE_LOCK_TIMEOUT_KEY - end - - def worker_class_run_lock_expiration - worker_class_expiration_for RUN_LOCK_TIMEOUT_KEY - end - - def worker_class - @worker_class ||= SidekiqUniqueJobs.worker_class_constantize(item[CLASS_KEY]) - end - - private - - def worker_class_expiration_for(key) - return unless worker_class.respond_to?(:get_sidekiq_options) - worker_class.get_sidekiq_options[key] - end - - attr_reader :item - end - - class RunLockTimeoutCalculator < TimeoutCalculator - def seconds - @seconds ||= ( - worker_class_run_lock_expiration || - SidekiqUniqueJobs.config.default_run_lock_expiration - ).to_i - end - end - - class QueueLockTimeoutCalculator < TimeoutCalculator - def seconds - queue_lock_expiration + time_until_scheduled - end - - def queue_lock_expiration - @queue_lock_expiration ||= - ( - worker_class_queue_lock_expiration || - SidekiqUniqueJobs.config.default_queue_lock_expiration - ).to_i - end - end -end diff --git a/lib/sidekiq_unique_jobs/unlockable.rb b/lib/sidekiq_unique_jobs/unlockable.rb index 403c5e92e..54fc25c02 100644 --- a/lib/sidekiq_unique_jobs/unlockable.rb +++ b/lib/sidekiq_unique_jobs/unlockable.rb @@ -5,21 +5,16 @@ module Unlockable module_function def unlock(item) - return unless item[UNIQUE_DIGEST_KEY] - unlock_by_key(item[UNIQUE_DIGEST_KEY], item[JID_KEY]) + SidekiqUniqueJobs::UniqueArgs.digest(item) + lock = SidekiqUniqueJobs::Lock.new(item) + lock.unlock end - def unlock_by_key(unique_key, jid, redis_pool = nil) - return false unless Scripts::ReleaseLock.execute(redis_pool, unique_key, jid) - after_unlock(jid) - end - - def after_unlock(jid) - ensure_job_id_removed(jid) - end - - def ensure_job_id_removed(jid) - Sidekiq.redis { |conn| conn.hdel(SidekiqUniqueJobs::HASH_KEY, jid) } + def delete!(item) + SidekiqUniqueJobs::UniqueArgs.digest(item) + lock = SidekiqUniqueJobs::Lock.new(item) + lock.unlock + lock.delete! end def logger diff --git a/lib/sidekiq_unique_jobs/util.rb b/lib/sidekiq_unique_jobs/util.rb index 31e484d83..f29c49915 100644 --- a/lib/sidekiq_unique_jobs/util.rb +++ b/lib/sidekiq_unique_jobs/util.rb @@ -1,29 +1,31 @@ # frozen_string_literal: true module SidekiqUniqueJobs - module Util # rubocop:disable Metrics/ModuleLength + module Util COUNT = 'COUNT' DEFAULT_COUNT = 1_000 EXPIRE_BATCH_SIZE = 100 - MATCH = 'MATCH' - KEYS_METHOD = 'keys' - SCAN_METHOD = 'scan' + SCAN_METHOD = 'SCAN' SCAN_PATTERN = '*' extend self # rubocop:disable Style/ModuleFunction def keys(pattern = SCAN_PATTERN, count = DEFAULT_COUNT) - send("keys_by_#{redis_keys_method}", pattern, count) - end - - def unique_key(jid) - connection do |conn| - conn.hget(SidekiqUniqueJobs::HASH_KEY, jid) - end + connection { |conn| conn.scan_each(match: prefix(pattern), count: count).to_a } end + # Deletes unique keys from redis + # + # + # @param pattern [String] a pattern to scan for in redis + # @param count [Integer] the maximum number of keys to delete + # @param dry_run [Boolean] set to false to perform deletion, `true` or `false` + # @return [Boolean] report success + # @raise [SidekiqUniqueJobs::LockTimeout] when lock fails within configured timeout def del(pattern = SCAN_PATTERN, count = 0, dry_run = true) raise ArgumentError, 'Please provide a number of keys to delete greater than zero' if count.zero? + pattern = "#{pattern}:*" unless pattern.end_with?(':*') + logger.debug { "Deleting keys by: #{pattern}" } keys, time = timed { keys(pattern, count) } logger.debug { "#{keys.size} matching keys found in #{time} sec." } @@ -37,47 +39,8 @@ def del(pattern = SCAN_PATTERN, count = 0, dry_run = true) keys.size end - def unique_hash - connection do |conn| - conn.hgetall(SidekiqUniqueJobs::HASH_KEY) - end - end - - def expire # rubocop:disable Metrics/MethodLength - removed_keys = {} - connection do |conn| - cursor = '0' - loop do - cursor, jobs = get_jobs(conn, cursor) - jobs.each do |job_array| - jid, unique_key = job_array - - next if conn.get(unique_key) - conn.hdel(SidekiqUniqueJobs::HASH_KEY, jid) - removed_keys[jid] = unique_key - end - - break if cursor == '0' - end - end - - removed_keys - end - private - def get_jobs(conn, cursor) - conn.hscan(SidekiqUniqueJobs::HASH_KEY, [cursor, MATCH, SCAN_PATTERN, COUNT, EXPIRE_BATCH_SIZE]) - end - - def keys_by_scan(pattern, count) - connection { |conn| conn.scan_each(match: prefix(pattern), count: count).to_a } - end - - def keys_by_keys(pattern, _count) - connection { |conn| conn.keys(prefix(pattern)).to_a } - end - def batch_delete(keys) connection do |conn| keys.each_slice(500) do |chunk| @@ -122,14 +85,6 @@ def connection(&block) SidekiqUniqueJobs.connection(&block) end - def redis_version - SidekiqUniqueJobs.redis_version - end - - def redis_keys_method - (redis_version >= '2.8') ? SCAN_METHOD : KEYS_METHOD - end - def logger SidekiqUniqueJobs.logger end diff --git a/rails_example/Gemfile b/rails_example/Gemfile index 33e90377b..a62708412 100644 --- a/rails_example/Gemfile +++ b/rails_example/Gemfile @@ -7,7 +7,8 @@ ruby '2.4.1' gem 'bigdecimal' gem 'json' gem 'puma' -gem 'rails', '~> 5.0' +gem 'rails', '~> 5.1' +gem 'slim-rails' platforms :ruby do gem 'pg' @@ -24,7 +25,7 @@ end # gem 'sidekiq-unique-jobs' gem 'rack-protection' # , github: 'sinatra/rack-protection' gem 'sidekiq-unique-jobs', path: '../' -gem 'sinatra', github: 'sinatra/sinatra' +gem 'sinatra' # , github: 'sinatra/sinatra' group :development do gem 'foreman' diff --git a/rails_example/app/channels/appearance_channel.rb b/rails_example/app/channels/appearance_channel.rb index ee55c53d3..4e85d1316 100644 --- a/rails_example/app/channels/appearance_channel.rb +++ b/rails_example/app/channels/appearance_channel.rb @@ -13,7 +13,5 @@ def appear(data) current_user.appear on: data['appearing_on'] end - def away - current_user.away - end + delegate :away, to: :current_user end diff --git a/rails_example/app/controllers/work_controller.rb b/rails_example/app/controllers/work_controller.rb index 45e61f9eb..f4000970c 100644 --- a/rails_example/app/controllers/work_controller.rb +++ b/rails_example/app/controllers/work_controller.rb @@ -1,6 +1,10 @@ # frozen_string_literal: true class WorkController < ApplicationController + def index + render :index + end + def duplicate_simple 4.times { SimpleWorker.perform_async(unique_argument) } @@ -33,9 +37,18 @@ def duplicate_with_args def duplicate_while_executing 4.times { WhileExecutingWorker.perform_async(1, 2) } + + redirect_to '/sidekiq' + end + + def duplicate_while_executing_with_timeout + 4.times { WhileExecutingWithTimeoutWorker.perform_async(1, 2) } + redirect_to '/sidekiq' end + private + def unique_argument params[:id] end diff --git a/rails_example/app/views/layouts/application.html.erb b/rails_example/app/views/layouts/application.html.erb deleted file mode 100644 index fcff7fa6a..000000000 --- a/rails_example/app/views/layouts/application.html.erb +++ /dev/null @@ -1,15 +0,0 @@ - - - - RailsExample - <%= stylesheet_link_tag 'application', media: 'all' %> - <%= javascript_include_tag 'application' %> - <%= csrf_meta_tags %> - - -<%= link_to 'Duplicate', duplicate_work_path %> - -<%= yield %> - - - diff --git a/rails_example/app/views/layouts/application.html.slim b/rails_example/app/views/layouts/application.html.slim new file mode 100644 index 000000000..8feca530b --- /dev/null +++ b/rails_example/app/views/layouts/application.html.slim @@ -0,0 +1,25 @@ +doctype html +html + head + title RailsExample + / = stylesheet_link_tag 'application', media: 'all' + / = javascript_include_tag 'application' + = csrf_meta_tags + body + ul#top-menu + li.menu-item + = link_to 'Duplicate Simple', work_duplicate_simple_path + li.menu-item + = link_to 'Duplicate Slow', work_duplicate_slow_path + li.menu-item + = link_to 'Duplicate Nested', work_duplicate_nested_path + li.menu-item + = link_to 'Duplicate WithoutArgs', work_duplicate_without_args_path + li.menu-item + = link_to 'Duplicate WithArgs', work_duplicate_with_args_path + li.menu-item + = link_to 'Duplicate WhileExecuting', work_duplicate_while_executing_path + li.menu-item + = link_to 'Duplicate WhileExecutingWithTimeout', work_duplicate_while_executing_with_timeout_path + + = yield diff --git a/rails_example/app/views/work/index.html.slim b/rails_example/app/views/work/index.html.slim new file mode 100644 index 000000000..e69de29bb diff --git a/rails_example/app/workers/while_executing_with_timeout_worker.rb b/rails_example/app/workers/while_executing_with_timeout_worker.rb new file mode 100644 index 000000000..5c3e0ed13 --- /dev/null +++ b/rails_example/app/workers/while_executing_with_timeout_worker.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class WhileExecutingWithTimeoutWorker + include Sidekiq::Worker + + sidekiq_options unique: :while_executing, lock_timeout: 5, unique_args: ->(args) { args.second } + + def perform(one, two) + Sidekiq::Logging.with_context(self.class.name) do + logger.info { "#{__method__}(#{one}, #{two})" } + end + sleep 10 + end +end diff --git a/rails_example/config/routes.rb b/rails_example/config/routes.rb index 34e808264..3d425925e 100644 --- a/rails_example/config/routes.rb +++ b/rails_example/config/routes.rb @@ -3,9 +3,14 @@ Rails.application.routes.draw do require 'sidekiq/web' mount Sidekiq::Web, at: '/sidekiq' - get 'work/duplicate_simple' => 'work#duplicate_simple' - get 'work/duplicate_nested' => 'work#duplicate_nested' - get 'work/duplicate_without_args' => 'work#duplicate_without_args' - get 'work/duplicate_with_args' => 'work#duplicate_with_args' - get 'work/duplicate_while_executing' => 'work#duplicate_while_executing' + + get 'work' => 'work#index' + + get 'work/duplicate_simple' => 'work#duplicate_simple' + get 'work/duplicate_slow' => 'work#duplicate_slow' + get 'work/duplicate_nested' => 'work#duplicate_nested' + get 'work/duplicate_without_args' => 'work#duplicate_without_args' + get 'work/duplicate_with_args' => 'work#duplicate_with_args' + get 'work/duplicate_while_executing' => 'work#duplicate_while_executing' + get 'work/duplicate_while_executing_with_timeout' => 'work#duplicate_while_executing_with_timeout' end diff --git a/rails_example/spec/controllers/work_controller_mock_spec.rb b/rails_example/spec/controllers/work_controller_mock_spec.rb index 3901dd25b..14ab4da99 100644 --- a/rails_example/spec/controllers/work_controller_mock_spec.rb +++ b/rails_example/spec/controllers/work_controller_mock_spec.rb @@ -4,21 +4,7 @@ MOCK_REDIS = MockRedis.new -describe WorkController, 'with mock redis' do - before do - MOCK_REDIS.keys.each do |key| - MOCK_REDIS.del(key) - end - - Sidekiq::Queues.clear_all - Sidekiq::Worker.clear_all - - SidekiqUniqueJobs.configure do |config| - config.redis_test_mode = :mock - end - allow(Sidekiq).to receive(:redis).and_yield(MOCK_REDIS) - end - +describe WorkController, 'with mock redis', redis: :mock_redis do describe 'GET /work/duplicate_simple' do context 'when test mode is fake', sidekiq: :fake do specify do @@ -29,11 +15,19 @@ end end - context 'when test mode is disabled', sidekiq: :disable do + context 'when test mode is disabled' do + let(:expected_keys) do + %w[ + uniquejobs:5f0092e13b3956c663a91d0d05d10a4b:EXISTS + uniquejobs:5f0092e13b3956c663a91d0d05d10a4b:GRABBED + ] + end + specify do get :duplicate_simple, params: { id: 11 } Sidekiq.redis do |conn| - expect(conn.keys.size).to eq(2) + expect(conn.keys).to match_array(expected_keys) + expect(conn.keys.size).to eq(3) expect(conn.llen('queue:default')).to eq(0) end end @@ -43,7 +37,7 @@ specify do get :duplicate_simple, params: { id: 12 } Sidekiq.redis do |conn| - expect(c).to be_a(MockRedis) + expect(conn).to be_a(MockRedis) expect(conn.keys.size).to eq(0) expect(conn.llen('queue:default')).to eq(0) end @@ -66,12 +60,12 @@ end end - context 'when test mode is disabled', sidekiq: :disable do + context 'when test mode is disabled' do specify do get :duplicate_nested, params: { id: 21 } Sidekiq.redis do |conn| - expect(c).to be_a(MockRedis) + expect(conn).to be_a(MockRedis) expect(conn.keys.size).to eq(0) expect(conn.llen('queue:default')).to eq(0) end @@ -83,7 +77,7 @@ get :duplicate_nested, params: { id: 22 } Sidekiq.redis do |conn| - expect(c).to be_a(MockRedis) + expect(conn).to be_a(MockRedis) expect(conn.keys.size).to eq(0) expect(conn.llen('queue:default')).to eq(0) end diff --git a/rails_example/spec/controllers/work_controller_spec.rb b/rails_example/spec/controllers/work_controller_spec.rb index 50ca51476..1c321e65a 100644 --- a/rails_example/spec/controllers/work_controller_spec.rb +++ b/rails_example/spec/controllers/work_controller_spec.rb @@ -2,7 +2,7 @@ require 'rails_helper' -describe WorkController, 'with real redis' do +describe WorkController, 'with real redis', redis: :redis, redis_db: 1 do before(:each) do SidekiqUniqueJobs.configure do |config| config.redis_test_mode = :redis @@ -22,7 +22,7 @@ end end - context 'when test mode is disabled', sidekiq: :disable do + context 'when test mode is disabled' do specify do get :duplicate_simple, params: { id: 41 } Sidekiq.redis do |conn| @@ -56,7 +56,7 @@ end end - context 'when test mode is disabled', sidekiq: :disable do + context 'when test mode is disabled' do specify do get :duplicate_nested, params: { id: 35 } diff --git a/rails_example/spec/rails_helper.rb b/rails_example/spec/rails_helper.rb index 6f0166596..91ea59a0b 100644 --- a/rails_example/spec/rails_helper.rb +++ b/rails_example/spec/rails_helper.rb @@ -10,6 +10,7 @@ require 'pry-byebug' require 'sidekiq_unique_jobs/testing' +require_relative '../../spec/support/sidekiq_meta' Dir[Rails.root.join('spec/support/**/*.rb')].each { |f| require f } ActiveRecord::Migration.maintain_test_schema! diff --git a/rails_example/spec/support/sidekiq_meta.rb b/rails_example/spec/support/sidekiq_meta.rb deleted file mode 100644 index e04bec37f..000000000 --- a/rails_example/spec/support/sidekiq_meta.rb +++ /dev/null @@ -1,14 +0,0 @@ -# frozen_string_literal: true - -RSpec.configure do |config| - config.before(:each) do |example| - if (sidekiq = example.metadata[:sidekiq]) - sidekiq = :fake if sidekiq == true - Sidekiq::Testing.send("#{sidekiq}!") - end - end - - config.after(:each) do |example| - Sidekiq::Testing.disable! unless example.metadata[:sidekiq].nil? - end -end diff --git a/redis/acquire_lock.lua b/redis/acquire_lock.lua deleted file mode 100644 index afdf4c547..000000000 --- a/redis/acquire_lock.lua +++ /dev/null @@ -1,19 +0,0 @@ -local unique_key = KEYS[1] -local job_id = ARGV[1] -local expires = ARGV[2] -local stored_jid = redis.pcall('get', unique_key) - -if stored_jid then - if stored_jid == job_id then - return 1 - else - return 0 - end -end - -if redis.pcall('set', unique_key, job_id, 'nx', 'ex', expires) then - redis.pcall('hsetnx', 'uniquejobs', job_id, unique_key) - return 1 -else - return 0 -end diff --git a/redis/exists_or_create.lua b/redis/exists_or_create.lua new file mode 100644 index 000000000..bd78488c8 --- /dev/null +++ b/redis/exists_or_create.lua @@ -0,0 +1,29 @@ +-- redis.replicate_commands(); + +local exists_key = KEYS[1] +local grabbed_key = KEYS[2] +local available_key = KEYS[3] +local current_jid = ARGV[1] +local expiration = tonumber(ARGV[2]) +local persisted_jid = redis.call('GETSET', exists_key, current_jid) + +if persisted_jid then + redis.log(redis.LOG_DEBUG, "exists_or_create.lua - returning persisted_jid : " .. persisted_jid) + return persisted_jid +else + redis.call('EXPIRE', exists_key, 10) + redis.call('DEL', grabbed_key) + redis.call('DEL', available_key) + redis.log(redis.LOG_DEBUG, "exists_or_create.lua - pushing available_key : " .. available_key .. " with jid : " .. current_jid) + redis.call('RPUSH', available_key, current_jid) + + redis.call('PERSIST', exists_key) + + if expiration then + redis.log(redis.LOG_DEBUG, "exists_or_create.lua - expiring locks in : " .. expiration) + redis.call('EXPIRE', available_key, expiration) + redis.call('EXPIRE', exists_key, expiration) + end + + return current_jid +end diff --git a/redis/release_lock.lua b/redis/release_lock.lua deleted file mode 100644 index f00c7833d..000000000 --- a/redis/release_lock.lua +++ /dev/null @@ -1,16 +0,0 @@ -local unique_key = KEYS[1] -local job_id = ARGV[1] -local stored_jid = redis.pcall('get', unique_key) - -if stored_jid then - if stored_jid == job_id or stored_jid == '2' then - redis.pcall('del', unique_key) - redis.pcall('hdel', 'uniquejobs', job_id) - return 1 - else - return 0 - end -else - return -1 -end - diff --git a/redis/release_stale_locks.lua b/redis/release_stale_locks.lua new file mode 100644 index 000000000..1f6068057 --- /dev/null +++ b/redis/release_stale_locks.lua @@ -0,0 +1,89 @@ +redis.replicate_commands(); + +local exists_key = KEYS[1] +local grabbed_key = KEYS[2] +local available_key = KEYS[3] +local lock_key = KEYS[4] + +local expires_in = tonumber(ARGV[1]) +local stale_client_timeout = tonumber(ARGV[2]) +local expiration = tonumber(ARGV[3]) + +local function current_time() + local time = redis.call('time') + local s = time[1] + local ms = time[2] + local number = tonumber((s .. '.' .. ms)) + + return number +end + +local hgetall = function (key) + local bulk = redis.call('HGETALL', key) + local result = {} + local nextkey + for i, v in ipairs(bulk) do + if i % 2 == 1 then + nextkey = v + else + result[nextkey] = v + end + end + return result +end + +local cached_current_time = current_time() +redis.log(redis.LOG_DEBUG, "release_stale_locks.lua - started at : " .. cached_current_time) + +local my_lock_expires_at = cached_current_time + expires_in + 1 +redis.log(redis.LOG_DEBUG, "release_stale_locks.lua - my_lock_expires_at: " .. my_lock_expires_at) + +if not redis.call('SETNX', lock_key, my_lock_expires_at) then + -- Check if expired + local other_lock_expires_at = tonumber(redis.call('GET', lock_key)) + redis.log(redis.LOG_DEBUG, "release_stale_locks.lua - other_lock_expires_at: " .. other_lock_expires_at) + + if other_lock_expires_at < cached_current_time then + local old_expires_at = tonumber(redis.call('GETSET', lock_key, my_lock_expires_at)) + redis.log(redis.LOG_DEBUG, "release_stale_locks.lua - old_expires_at: " .. old_expires_at) + + -- Check if another client started cleanup yet. If not, + -- then we now have the lock. + if not old_expires_at == other_lock_expires_at then + redis.log(redis.LOG_DEBUG, "release_stale_locks.lua -could not retrieve lock: exiting 0") + return 0 + end + end +end + +local keys = hgetall(grabbed_key) +for key, locked_at in pairs(keys) do + local timed_out_at = tonumber(locked_at) + stale_client_timeout + redis.log(redis.LOG_DEBUG, + "release_stale_locks.lua - processing: " .. grabbed_key .. " key: " .. key .. " locked_at: " .. locked_at + ) + + if timed_out_at < current_time() then + redis.log(redis.LOG_DEBUG, "HDEL " .. grabbed_key .. ":" .. key) + redis.call('HDEL', grabbed_key, key) + redis.log(redis.LOG_DEBUG, "LPUSH " .. available_key .. ":" .. key) + redis.call('LPUSH', available_key, key) + + if expiration then + redis.log(redis.LOG_DEBUG, "release_stale_locks.lua - EXPIRE " .. available_key .. " with " .. expiration) + redis.call('EXPIRE', available_key, expiration) + redis.log(redis.LOG_DEBUG, "release_stale_locks.lua - EXPIRE " .. exists_key .. " with " .. expiration) + redis.call('EXPIRE', exists_key, expiration) + end + end +end + +-- Make sure not to delete the lock in case someone else already expired +-- our lock, with one second in between to account for some lag. +if my_lock_expires_at > (current_time() - 1) then + redis.log(redis.LOG_DEBUG, "release_stale_locks.lua - DEL " .. lock_key) + redis.call('DEL', lock_key) +end + +redis.log(redis.LOG_DEBUG, "release_stale_locks.lua - comleted at : " .. current_time()) +return 1 diff --git a/redis/synchronize.lua b/redis/synchronize.lua deleted file mode 100644 index b7fc0a70e..000000000 --- a/redis/synchronize.lua +++ /dev/null @@ -1,16 +0,0 @@ -local unique_key = KEYS[1] -local time = ARGV[1] -local expires = ARGV[2] - -if redis.pcall('set', unique_key, time + expires, 'nx', 'ex', expires) then - return 1 -end - -local stored_time = redis.pcall('get', unique_key) -if stored_time and stored_time < time then - if redis.pcall('set', unique_key, time + expires, 'xx', 'ex', expires) then - return 1 - end -end - -return 0 diff --git a/sidekiq-unique-jobs.gemspec b/sidekiq-unique-jobs.gemspec index ac052cb85..b624f7200 100644 --- a/sidekiq-unique-jobs.gemspec +++ b/sidekiq-unique-jobs.gemspec @@ -23,10 +23,12 @@ Gem::Specification.new do |spec| spec.add_dependency 'sidekiq', '>= 4.0', '<= 6.0' spec.add_dependency 'thor', '~> 0' + spec.add_development_dependency 'bundler', '~> 1.5' spec.add_development_dependency 'rspec', '~> 3.1' spec.add_development_dependency 'rake', '~> 12.0' spec.add_development_dependency 'timecop', '~> 0.8' spec.add_development_dependency 'yard', '~> 0.9' spec.add_development_dependency 'gem-release', '~> 0.7' - spec.add_development_dependency 'codeclimate-test-reporter', '~> 1.0', '>= 1.0.8' + spec.add_development_dependency 'mock_redis', '~> 0.17.3' + spec.add_development_dependency 'awesome_print', '~> 1.8' end diff --git a/spec/integration/client/middleware_spec.rb b/spec/integration/client/middleware_spec.rb new file mode 100644 index 000000000..42b17b90c --- /dev/null +++ b/spec/integration/client/middleware_spec.rb @@ -0,0 +1,333 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'sidekiq/worker' +require 'sidekiq-unique-jobs' +require 'rspec/wait' + +RSpec.shared_examples_for 'unique client middleware' do + describe 'when a job is already scheduled' do + it 'processes jobs properly' do + jid = NotifyWorker.perform_in(1, 183, 'xxxx') + expect(jid).not_to eq(nil) + Sidekiq.redis do |conn| + expect(conn.zcard('schedule')).to eq(1) + expected = %w[uniquejobs:6e47d668ad22db2a3ba0afd331514ce2:EXISTS] + + expect(conn.keys).to include(*expected) + end + Sidekiq::Scheduled::Enq.new.enqueue_jobs + + Sidekiq::Simulator.process_queue(:notify_worker) do + sleep 1 + Sidekiq.redis do |conn| + wait(10).for { conn.llen('queue:notify_worker') }.to eq(0) + end + end + end + + it 'rejects nested subsequent jobs with the same arguments' do + expect(SimpleWorker.perform_async(1)).not_to eq(nil) + expect(SimpleWorker.perform_async(1)).to eq(nil) + expect(SpawnSimpleWorker.perform_async(1)).not_to eq(nil) + + Sidekiq.redis do |conn| + expect(conn.llen('queue:default')).to eq(1) + expect(conn.llen('queue:not_default')).to eq(1) + end + + Sidekiq::Simulator.process_queue(:not_default) do + sleep(1) + Sidekiq.redis do |conn| + expect(conn.llen('queue:default')).to eq(1) + wait(10).for { conn.llen('queue:not_default') }.to eq(0) + expect(conn.llen('queue:default')).to eq(1) + end + end + + Sidekiq.redis do |conn| + expect(conn.llen('queue:default')).to eq(1) + end + + Sidekiq::Simulator.process_queue(:default) do + sleep(1) + Sidekiq.redis do |conn| + expect(conn.llen('queue:not_default')).to eq(0) + wait(10).for { conn.llen('queue:default') }.to eq(0) + end + end + end + + it 'rejects new scheduled jobs with the same argument' do + MyUniqueJob.perform_in(3600, 1, 2) + expect(MyUniqueJob.perform_in(3600, 1, 2)).to eq(nil) + end + + it 'will run a job in real time with the same arguments' do + WhileExecutingJob.perform_in(3600, 1) + expect(WhileExecutingJob.perform_async(1)).not_to eq(nil) + end + + it 'schedules new jobs when arguments differ' do + [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20].each do |x| + MainJob.perform_in(x, x) + end + + Sidekiq.redis do |conn| + count = conn.zcount('schedule', -1, Time.now.to_f + 2 * 60) + expect(count).to eq(20) + end + end + + it 'schedules allows jobs to be scheduled ' do + class ShitClass + def self.do_it(_one) + # whatever + end + end + + [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20].each do |x| + ShitClass.delay_for(x, unique: :while_executing).do_it(1) + end + + Sidekiq.redis do |conn| + count = conn.zcount('schedule', -1, Time.now.to_f + 2 * 60) + expect(count).to eq(20) + end + end + end + + it 'does not push duplicate messages when configured for unique only' do + 10.times { MyUniqueJob.perform_async(1, 2) } + Sidekiq.redis do |conn| + expect(conn.llen('queue:customqueue')).to eq(1) + end + end + + it 'does not push duplicate messages when unique_args are filtered with a proc' do + 10.times { MyUniqueJobWithFilterProc.perform_async(1) } + Sidekiq.redis { |conn| expect(conn.llen('queue:customqueue')).to eq(1) } + + Sidekiq.redis(&:flushdb) + Sidekiq.redis { |conn| expect(conn.llen('queue:customqueue')).to eq(0) } + + 10.times do + Sidekiq::Client.push( + 'class' => MyUniqueJobWithFilterProc, + 'queue' => 'customqueue', + 'args' => [1, type: 'value', some: 'not used'], + ) + end + Sidekiq.redis { |conn| expect(conn.llen('queue:customqueue')).to eq(1) } + end + + it 'does not push duplicate messages when unique_args are filtered with a method' do + 10.times { MyUniqueJobWithFilterMethod.perform_async(1) } + Sidekiq.redis do |conn| + expect(conn.llen('queue:customqueue')).to eq(1) + end + Sidekiq.redis(&:flushdb) + Sidekiq.redis { |conn| expect(conn.llen('queue:customqueue')).to eq(0) } + 10.times do + Sidekiq::Client.push( + 'class' => MyUniqueJobWithFilterMethod, + 'queue' => 'customqueue', + 'args' => [1, type: 'value', some: 'not used'], + ) + end + Sidekiq.redis { |conn| expect(conn.llen('queue:customqueue')).to eq(1) } + end + + it 'does push duplicate messages to different queues' do + Sidekiq::Client.push('class' => MyUniqueJob, 'queue' => 'customqueue', 'args' => [1, 2]) + Sidekiq::Client.push('class' => MyUniqueJob, 'queue' => 'customqueue2', 'args' => [1, 2]) + Sidekiq.redis do |conn| + expect(conn.llen('queue:customqueue')).to eq 1 + expect(conn.llen('queue:customqueue2')).to eq 1 + end + end + + it 'does not queue duplicates when when calling delay' do + 10.times { PlainClass.delay(unique: :until_executed, queue: 'customqueue').run(1) } + Sidekiq.redis do |conn| + expect(conn.llen('queue:customqueue')).to eq(1) + end + end + + it 'does not schedule duplicates when calling perform_in' do + 10.times { MyUniqueJob.perform_in(60, 1, 2) } + Sidekiq.redis do |conn| + expect(conn.zcount('schedule', -1, Time.now.to_f + 2 * 60)) + .to eq(1) + end + end + + it 'enqueues previously scheduled job' do + jid = WhileExecutingJob.perform_in(60 * 60, 1, 2) + item = { 'class' => WhileExecutingJob, 'queue' => 'customqueue', 'args' => [1, 2], 'jid' => jid } + + # time passes and the job is pulled off the schedule: + Sidekiq::Client.push(item) + + Sidekiq.redis do |conn| + expect(conn.llen('queue:customqueue')).to eq 1 + end + end + + context 'when expiration is configured' do + it 'sets that expiration on the key' do + item = { 'class' => ExpiringJob, 'queue' => 'customqueue', 'args' => [1, 2] } + Sidekiq::Client.push(item) + + Sidekiq.redis do |conn| + expect(conn.llen('queue:customqueue')).to eq(1) + + conn.keys('uniquejobs:*').each do |key| + next if key.end_with?(':GRABBED') + + expect(conn.ttl(key)).to be_within(1).of(ExpiringJob.get_sidekiq_options['lock_expiration']) + end + end + end + end + + context 'when class is not unique' do + it 'pushes duplicate messages' do + 10.times do + Sidekiq::Client.push('class' => CustomQueueJob, 'queue' => 'customqueue', 'args' => [1, 2]) + end + + Sidekiq.redis do |conn| + expect(conn.llen('queue:customqueue')).to eq(10) + end + end + end + + describe 'when unique_args is defined' do + context 'when filter method is defined' do + it 'pushes no duplicate messages' do + expect(CustomQueueJobWithFilterMethod).to respond_to(:args_filter) + expect(CustomQueueJobWithFilterMethod.get_sidekiq_options['unique_args']).to eq :args_filter + + (0..10).each do |i| + Sidekiq::Client.push( + 'class' => CustomQueueJobWithFilterMethod, + 'queue' => 'customqueue', + 'args' => [1, i], + ) + end + + Sidekiq.redis do |conn| + expect(conn.llen('queue:customqueue')).to eq(1) + end + end + end + + context 'when filter proc is defined' do + it 'pushes no duplicate messages' do + expect(CustomQueueJobWithFilterProc.get_sidekiq_options['unique_args']).to be_a(Proc) + + 100.times do + Sidekiq::Client.push( + 'class' => CustomQueueJobWithFilterProc, + 'queue' => 'customqueue', + 'args' => [1, { random: rand, name: 'foobar' }], + ) + end + + Sidekiq.redis do |conn| + expect(conn.llen('queue:customqueue')).to eq(1) + end + end + end + + context 'when unique_on_all_queues is set' do + it 'pushes no duplicate messages on other queues' do + item = { 'class' => UniqueOnAllQueuesJob, 'args' => [1, 2] } + Sidekiq::Client.push(item.merge('queue' => 'customqueue')) + Sidekiq::Client.push(item.merge('queue' => 'customqueue2')) + Sidekiq.redis do |conn| + expect(conn.llen('queue:customqueue')).to eq(1) + expect(conn.llen('queue:customqueue2')).to eq(0) + end + end + end + + context 'when unique_on_all_queues is set' do + it 'does not push duplicate messages for other workers' do + item_one = { + 'queue' => 'customqueue1', + 'class' => UniqueAcrossWorkersJob, + 'unique_across_workers' => true, + 'args' => [1, 2], + } + + item_two = { + 'queue' => 'customqueue1', + 'class' => MyUniqueJob, + 'unique_across_workers' => true, + 'args' => [1, 2], + } + + Sidekiq::Client.push(item_one) + Sidekiq::Client.push(item_two) + Sidekiq.redis do |conn| + expect(conn.llen('queue:customqueue1')).to eq(1) + end + end + end + end + + # TODO: If anyone know of a better way to check that the expiration for scheduled + # jobs are set around the same time as the scheduled job itself feel free to improve. + it 'expires the digest when a scheduled job is scheduled at' do + expected_expires_at = + (Time.at(Time.now.to_i + 15 * 60) - Time.now.utc) + + SidekiqUniqueJobs.config.default_queue_lock_expiration.to_f + + MyUniqueJob.perform_in(expected_expires_at, 'mika', 'hel') + + Sidekiq.redis do |conn| + conn.keys('uniquejobs:*').each do |key| + next if key.end_with?(':GRABBED') + expect(conn.ttl(key)).to be_within(10).of(9_900) + end + end + end + + it 'logs duplicate payload when config turned on' do + expect(Sidekiq.logger).to receive(:warn).with(/^payload is not unique/) + UntilExecutedJob.sidekiq_options log_duplicate_payload: true + + 2.times do + Sidekiq::Client.push('class' => UntilExecutedJob, 'queue' => 'customqueue', 'args' => [1, 2]) + end + + Sidekiq.redis do |conn| + expect(conn.llen('queue:customqueue')).to eq 1 + end + + UntilExecutedJob.sidekiq_options log_duplicate_payload: false + end + + it 'does not log duplicate payload when config turned off' do + expect(SidekiqUniqueJobs.logger).to_not receive(:warn).with(/^payload is not unique/) + + UntilExecutedJob.sidekiq_options log_duplicate_payload: false + + 2.times do + Sidekiq::Client.push('class' => UntilExecutedJob, 'queue' => 'customqueue', 'args' => [1, 2]) + end + Sidekiq.redis do |conn| + expect(conn.llen('queue:customqueue')).to eq 1 + end + end +end + +RSpec.describe SidekiqUniqueJobs::Client::Middleware, 'with Redis', redis: :redis, redis_db: 1 do + it_behaves_like 'unique client middleware' +end + +# RSpec.xdescribe SidekiqUniqueJobs::Client::Middleware, 'with MockRedis', redis: :mock_redis do +# it_behaves_like 'unique client middleware' +# end diff --git a/spec/integration/sidekiq_testing_fake_spec.rb b/spec/integration/sidekiq_testing_fake_spec.rb new file mode 100644 index 000000000..1e2e01e80 --- /dev/null +++ b/spec/integration/sidekiq_testing_fake_spec.rb @@ -0,0 +1,131 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'sidekiq/worker' +require 'sidekiq-unique-jobs' +require 'sidekiq/scheduled' + +RSpec.shared_examples 'Sidekiq::Testing.fake!' do + context 'with unique worker' do + it 'does not push duplicate messages' do + param = 'work' + expect(UntilExecutedJob.jobs.size).to eq(0) + expect(UntilExecutedJob.perform_async(param)).to_not be_nil + expect(UntilExecutedJob.jobs.size).to eq(1) + expect(UntilExecutedJob.perform_async(param)).to be_nil + expect(UntilExecutedJob.jobs.size).to eq(1) + end + + it 'unlocks jobs after draining a worker' do + param = 'work 2' + param2 = 'more work 2' + expect(UntilExecutedJob.jobs.size).to eq(0) + expect(UntilExecutedJob.perform_async(param)).not_to be_nil + expect(UntilExecutedJob.perform_async(param2)).not_to be_nil + expect(UntilExecutedJob.jobs.size).to eq(2) + UntilExecutedJob.drain + expect(UntilExecutedJob.jobs.size).to eq(0) + expect(UntilExecutedJob.perform_async(param)).not_to be_nil + expect(UntilExecutedJob.perform_async(param2)).not_to be_nil + expect(UntilExecutedJob.jobs.size).to eq(2) + end + + it 'unlocks a single job when calling perform_one' do + param = 'work 3' + param2 = 'more work 3' + expect(UntilExecutedJob.jobs.size).to eq(0) + expect(UntilExecutedJob.perform_async(param)).not_to be_nil + expect(UntilExecutedJob.perform_async(param2)).not_to be_nil + expect(UntilExecutedJob.jobs.size).to eq(2) + UntilExecutedJob.perform_one + expect(UntilExecutedJob.jobs.size).to eq(1) + expect(UntilExecutedJob.perform_async(param2)).to be_nil + expect(UntilExecutedJob.jobs.size).to eq(1) + expect(UntilExecutedJob.perform_async(param)).not_to be_nil + expect(UntilExecutedJob.jobs.size).to eq(2) + end + + it 'unlocks jobs cleared from a single worker' do + param = 'work 4' + param2 = 'more work 4' + expect(UntilExecutedJob.jobs.size).to eq(0) + expect(AnotherUniqueJob.jobs.size).to eq(0) + UntilExecutedJob.perform_async(param) + UntilExecutedJob.perform_async(param2) + AnotherUniqueJob.perform_async(param) + expect(UntilExecutedJob.jobs.size).to eq(2) + expect(AnotherUniqueJob.jobs.size).to eq(1) + UntilExecutedJob.clear + expect(UntilExecutedJob.jobs.size).to eq(0) + expect(AnotherUniqueJob.jobs.size).to eq(1) + UntilExecutedJob.perform_async(param) + UntilExecutedJob.perform_async(param2) + AnotherUniqueJob.perform_async(param) + expect(UntilExecutedJob.jobs.size).to eq(2) + expect(AnotherUniqueJob.jobs.size).to eq(1) + end + + it 'handles clearing an empty worker queue' do + param = 'work 5' + UntilExecutedJob.perform_async(param) + UntilExecutedJob.drain + expect(UntilExecutedJob.jobs.size).to eq(0) + expect { UntilExecutedJob.clear }.not_to raise_error + end + + it 'unlocks jobs when all workers are cleared' do + param = 'work 6' + expect(UntilExecutedJob.jobs.size).to eq(0) + expect(AnotherUniqueJob.jobs.size).to eq(0) + UntilExecutedJob.perform_async(param) + AnotherUniqueJob.perform_async(param) + expect(UntilExecutedJob.jobs.size).to eq(1) + expect(AnotherUniqueJob.jobs.size).to eq(1) + Sidekiq::Worker.clear_all + expect(UntilExecutedJob.jobs.size).to eq(0) + expect(AnotherUniqueJob.jobs.size).to eq(0) + UntilExecutedJob.perform_async(param) + expect(UntilExecutedJob.jobs.size).to eq(1) + AnotherUniqueJob.perform_async(param) + expect(AnotherUniqueJob.jobs.size).to eq(1) + end + + it 'handles clearing all workers when there are no jobs' do + param = 'work 7' + UntilExecutedJob.perform_async(param) + AnotherUniqueJob.perform_async(param) + Sidekiq::Worker.clear_all + expect(UntilExecutedJob.jobs.size).to eq(0) + expect(AnotherUniqueJob.jobs.size).to eq(0) + expect { Sidekiq::Worker.jobs.size }.not_to raise_error + end + + it 'adds the unique_digest to the message' do + param = 'hash' + item = { 'class' => 'UntilExecutedJob', 'queue' => 'working', 'args' => [param] } + digest = SidekiqUniqueJobs::UniqueArgs.digest(item) + expect(UntilExecutedJob.perform_async(param)).to_not be_nil + expect(UntilExecutedJob.jobs.size).to eq(1) + expect(UntilExecutedJob.jobs.last['unique_digest']).to eq(digest) + end + end + + context 'with non-unique worker' do + it 'pushes duplicates messages' do + param = 'work' + expect(MyJob.jobs.size).to eq(0) + MyJob.perform_async(param) + expect(MyJob.jobs.size).to eq(1) + MyJob.perform_async(param) + expect(MyJob.jobs.size).to eq(2) + end + end +end + +RSpec.describe Sidekiq::Testing, 'with Redis', redis: :redis, redis_db: 12, sidekiq: :fake do + it_behaves_like 'Sidekiq::Testing.fake!' +end + +RSpec.describe Sidekiq::Testing, 'with MockRedis', redis: :mock_redis, sidekiq: :fake do + it_behaves_like 'Sidekiq::Testing.fake!' +end diff --git a/spec/integration/sidekiq_testing_inline_spec.rb b/spec/integration/sidekiq_testing_inline_spec.rb new file mode 100644 index 000000000..a3cb2fb1f --- /dev/null +++ b/spec/integration/sidekiq_testing_inline_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'sidekiq/worker' +require 'sidekiq-unique-jobs' +require 'sidekiq/scheduled' + +RSpec.shared_examples 'Sidekiq::Testing.inline!' do + it 'once the job is completed allows to run another one' do + expect(TestClass).to receive(:run).with('plosibubb').twice + InlineWorker.perform_async('plosibubb') + InlineWorker.perform_async('plosibubb') + end + + describe 'when a job is set to run once in 10 minutes' do + context 'when spammed' do + it 'only allows 1 call per 10 minutes' do + expect(TestClass).to receive(:run).with(1).once + 100.times do + UntilTimeoutJob.perform_async(1) + end + end + end + + context 'with different arguments' do + it 'only allows 1 call per 10 minutes' do + expect(TestClass).to receive(:run).with(9).once + 2.times do + UntilTimeoutJob.perform_async(9) + end + + expect(TestClass).to receive(:run).with(2).once + 2.times do + UntilTimeoutJob.perform_async(2) + end + end + end + end +end + +RSpec.describe Sidekiq::Testing, 'with Redis', redis: :redis, redis_db: 13, sidekiq: :inline do + it_behaves_like 'Sidekiq::Testing.inline!' +end + +RSpec.describe Sidekiq::Testing, 'with MockRedis', redis: :mock_redis, sidekiq: :inline do + it_behaves_like 'Sidekiq::Testing.inline!' +end diff --git a/spec/jobs/custom_queue_job.rb b/spec/jobs/custom_queue_job.rb index 60ef6d379..52c5bc04b 100644 --- a/spec/jobs/custom_queue_job.rb +++ b/spec/jobs/custom_queue_job.rb @@ -3,5 +3,7 @@ class CustomQueueJob include Sidekiq::Worker sidekiq_options queue: :customqueue - def perform(_); end + def perform(one, two) + [one, two] + end end diff --git a/spec/jobs/expiring_job.rb b/spec/jobs/expiring_job.rb index 79e7bd9b7..81215a7b6 100644 --- a/spec/jobs/expiring_job.rb +++ b/spec/jobs/expiring_job.rb @@ -2,5 +2,9 @@ class ExpiringJob include Sidekiq::Worker - sidekiq_options unique: :until_executed, unique_expiration: 10 * 60 + sidekiq_options unique: :until_executed, lock_expiration: 10 * 60 + + def perform(one, two) + [one, two] + end end diff --git a/spec/jobs/inline_worker.rb b/spec/jobs/inline_worker.rb index 3e75b83a1..3ded2a8f8 100644 --- a/spec/jobs/inline_worker.rb +++ b/spec/jobs/inline_worker.rb @@ -2,9 +2,9 @@ class InlineWorker include Sidekiq::Worker - sidekiq_options unique: :while_executing + sidekiq_options unique: :while_executing, lock_timeout: 5 - def perform(x) - TestClass.run(x) + def perform(one) + TestClass.run(one) end end diff --git a/spec/jobs/just_a_worker.rb b/spec/jobs/just_a_worker.rb index 1279b5c7d..16308ffdc 100644 --- a/spec/jobs/just_a_worker.rb +++ b/spec/jobs/just_a_worker.rb @@ -5,5 +5,7 @@ class JustAWorker sidekiq_options unique: :until_executed, queue: 'testqueue' - def perform; end + def perform(options = {}) + options + end end diff --git a/spec/jobs/long_running_job.rb b/spec/jobs/long_running_job.rb index b9c86b258..49ebdb8b7 100644 --- a/spec/jobs/long_running_job.rb +++ b/spec/jobs/long_running_job.rb @@ -3,6 +3,6 @@ class LongRunningJob include Sidekiq::Worker sidekiq_options queue: :customqueue, retry: true, unique: :until_and_while_executing, - run_lock_expiration: 7_200, retry_count: 10 + lock_expiration: 7_200, retry_count: 10 def perform(_one, _two); end end diff --git a/spec/jobs/long_running_run_lock_expiration_job.rb b/spec/jobs/long_running_run_lock_expiration_job.rb new file mode 100644 index 000000000..93cbf596e --- /dev/null +++ b/spec/jobs/long_running_run_lock_expiration_job.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class LongRunningRunLockExpirationJob + include Sidekiq::Worker + sidekiq_options queue: :customqueue, retry: true, unique: :until_and_while_executing, + run_lock_expiration: 3_600, retry_count: 10 + def perform(_one, _two); end +end diff --git a/spec/jobs/my_job.rb b/spec/jobs/my_job.rb index ce1faef77..13edb178e 100644 --- a/spec/jobs/my_job.rb +++ b/spec/jobs/my_job.rb @@ -2,7 +2,7 @@ class MyJob include Sidekiq::Worker - sidekiq_options queue: :working, retry: 1, backtrace: 10, unique: false + sidekiq_options queue: :working, retry: 1, backtrace: 10 sidekiq_retries_exhausted do |msg| logger.warn "Failed #{msg['class']} with #{msg['args']}: #{msg['error_message']}" diff --git a/spec/jobs/my_unique_job.rb b/spec/jobs/my_unique_job.rb index 87d12e5ce..adc163c84 100644 --- a/spec/jobs/my_unique_job.rb +++ b/spec/jobs/my_unique_job.rb @@ -2,12 +2,10 @@ class MyUniqueJob include Sidekiq::Worker - sidekiq_options( - queue: :customqueue, - retry: true, - unique: :until_executed, - unique_expiration: 7_200, - retry_count: 10, - ) - def perform(_one, _two); end + sidekiq_options queue: :customqueue, retry: true, retry_count: 10, + unique: :until_executed, lock_expiration: 7_200 + + def perform(one, two) + [one, two] + end end diff --git a/spec/jobs/plain_class.rb b/spec/jobs/plain_class.rb index f82724a9a..5a5b9aa7e 100644 --- a/spec/jobs/plain_class.rb +++ b/spec/jobs/plain_class.rb @@ -1,5 +1,11 @@ # frozen_string_literal: true class PlainClass - def run(_x); end + def self.run(one) + [one] + end + + def run(one) + [one] + end end diff --git a/spec/jobs/test_class.rb b/spec/jobs/test_class.rb index bb15d8a5c..fe72325ae 100644 --- a/spec/jobs/test_class.rb +++ b/spec/jobs/test_class.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true class TestClass - def self.run(_); end + def self.run(one) + one + end end diff --git a/spec/jobs/until_and_while_executing_job.rb b/spec/jobs/until_and_while_executing_job.rb index 5ae241113..20ca633ae 100644 --- a/spec/jobs/until_and_while_executing_job.rb +++ b/spec/jobs/until_and_while_executing_job.rb @@ -1,9 +1,11 @@ # frozen_string_literal: true -class UntilAndWhileExecuting +class UntilAndWhileExecutingJob include Sidekiq::Worker sidekiq_options queue: :working, unique: :until_and_while_executing - def perform; end + def perform(one) + [one] + end end diff --git a/spec/jobs/until_executed_2_job.rb b/spec/jobs/until_executed_2_job.rb new file mode 100644 index 000000000..067e25f39 --- /dev/null +++ b/spec/jobs/until_executed_2_job.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +# This class will lock until the job is successfully executed +# +# It will wait for 0 seconds to acquire a lock and it will expire the unique key after 2 seconds +class UntilExecuted2Job + include Sidekiq::Worker + sidekiq_options queue: :working, retry: 1, backtrace: 10 + sidekiq_options unique: :until_executed, lock_timeout: 0 + + sidekiq_retries_exhausted do |msg| + logger.warn "Failed #{msg['class']} with #{msg['args']}: #{msg['error_message']}" + end + + def perform(*) + # NO-OP + end + + def after_unlock + # NO OP + end +end diff --git a/spec/jobs/until_executed_job.rb b/spec/jobs/until_executed_job.rb index 3ec8ce2a5..d118d3d9f 100644 --- a/spec/jobs/until_executed_job.rb +++ b/spec/jobs/until_executed_job.rb @@ -1,15 +1,18 @@ # frozen_string_literal: true +# This class will lock until the job is successfully executed +# +# It will wait for 0 seconds to acquire a lock and it will expire the unique key after 2 seconds class UntilExecutedJob include Sidekiq::Worker sidekiq_options queue: :working, retry: 1, backtrace: 10 - sidekiq_options unique: :until_executed + sidekiq_options unique: :until_executed, lock_timeout: 0, lock_expiration: nil sidekiq_retries_exhausted do |msg| logger.warn "Failed #{msg['class']} with #{msg['args']}: #{msg['error_message']}" end - def perform(*) + def perform(one, two = nil) # NO-OP end diff --git a/spec/jobs/until_timeout_job.rb b/spec/jobs/until_timeout_job.rb index 3e8be0d62..a178a0704 100644 --- a/spec/jobs/until_timeout_job.rb +++ b/spec/jobs/until_timeout_job.rb @@ -2,9 +2,9 @@ class UntilTimeoutJob include Sidekiq::Worker - sidekiq_options unique: :until_timeout, - unique_expiration: 10 * 60 - def perform(x) - TestClass.run(x) + sidekiq_options unique: :until_timeout, lock_expiration: 10 * 60, lock_timeout: 10 + + def perform(one) + TestClass.run(one) end end diff --git a/spec/jobs/while_executing_job.rb b/spec/jobs/while_executing_job.rb index d169d7724..b3ce29ca1 100644 --- a/spec/jobs/while_executing_job.rb +++ b/spec/jobs/while_executing_job.rb @@ -8,7 +8,7 @@ class WhileExecutingJob logger.warn "Failed #{msg['class']} with #{msg['args']}: #{msg['error_message']}" end - def perform(_) + def perform(*) # NO OP end end diff --git a/spec/lib/sidekiq_unique_jobs/client/middleware_spec.rb b/spec/lib/sidekiq_unique_jobs/client/middleware_spec.rb deleted file mode 100644 index 745b307f7..000000000 --- a/spec/lib/sidekiq_unique_jobs/client/middleware_spec.rb +++ /dev/null @@ -1,345 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' -require 'sidekiq/worker' -require 'sidekiq-unique-jobs' -require 'rspec/wait' - -RSpec.describe SidekiqUniqueJobs::Client::Middleware do - def digest_for(item) - SidekiqUniqueJobs::UniqueArgs.digest(item) - end - - describe 'with real redis' do - describe 'when a job is already scheduled' do - it 'processes jobs properly' do - Sidekiq::Testing.disable! do - jid = NotifyWorker.perform_in(1, 183, 'xxxx') - expect(jid).not_to eq(nil) - Sidekiq.redis do |conn| - expect(conn.zcard('schedule')).to eq(1) - expected = %w[schedule uniquejobs:6e47d668ad22db2a3ba0afd331514ce2 uniquejobs] - expect(conn.keys).to match_array(expected) - end - sleep 1 - Sidekiq::Scheduled::Enq.new.enqueue_jobs - - Sidekiq.redis do |conn| - wait(10).for { conn.llen('queue:notify_worker') }.to eq(1) - end - - Sidekiq::Simulator.process_queue(:notify_worker) do - sleep 1 - Sidekiq.redis do |conn| - wait(10).for { conn.llen('queue:notify_worker') }.to eq(0) - end - end - end - end - - it 'rejects nested subsequent jobs with the same arguments' do - Sidekiq::Testing.disable! do - expect(SimpleWorker.perform_async(1)).not_to eq(nil) - expect(SimpleWorker.perform_async(1)).to eq(nil) - expect(SpawnSimpleWorker.perform_async(1)).not_to eq(nil) - - Sidekiq.redis do |conn| - expect(conn.llen('queue:default')).to eq(1) - expect(conn.llen('queue:not_default')).to eq(1) - end - - Sidekiq::Simulator.process_queue(:not_default) do - sleep(1) - Sidekiq.redis do |conn| - expect(conn.llen('queue:default')).to eq(1) - wait(10).for { conn.llen('queue:not_default') }.to eq(0) - expect(conn.llen('queue:default')).to eq(1) - end - end - - Sidekiq.redis do |conn| - expect(conn.llen('queue:default')).to eq(1) - end - - Sidekiq::Simulator.process_queue(:default) do - sleep(1) - Sidekiq.redis do |conn| - expect(conn.llen('queue:not_default')).to eq(0) - wait(10).for { conn.llen('queue:default') }.to eq(0) - end - end - end - end - - it 'rejects new scheduled jobs with the same argument' do - MyUniqueJob.perform_in(3600, 1) - expect(MyUniqueJob.perform_in(3600, 1)).to eq(nil) - end - - it 'will run a job in real time with the same arguments' do - WhileExecutingJob.perform_in(3600, 1) - expect(WhileExecutingJob.perform_async(1)).not_to eq(nil) - end - - it 'schedules new jobs when arguments differ' do - [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20].each do |x| - MainJob.perform_in(x, x) - end - - Sidekiq.redis do |conn| - count = conn.zcount('schedule', -1, Time.now.to_f + 2 * 60) - expect(count).to eq(20) - end - end - - it 'schedules allows jobs to be scheduled ' do - class ShitClass - def do_it(_arg) - # whatever - end - end - - [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20].each do |x| - ShitClass.delay_for(x, unique: :while_executing).do_it(1) - end - - Sidekiq.redis do |conn| - count = conn.zcount('schedule', -1, Time.now.to_f + 2 * 60) - expect(count).to eq(20) - end - end - end - - it 'does not push duplicate messages when configured for unique only' do - 10.times { MyUniqueJob.perform_async(1, 2) } - Sidekiq.redis do |conn| - expect(conn.llen('queue:customqueue')).to eq(1) - end - end - - it 'does not push duplicate messages when unique_args are filtered with a proc' do - 10.times { MyUniqueJobWithFilterProc.perform_async(1) } - Sidekiq.redis { |conn| expect(conn.llen('queue:customqueue')).to eq(1) } - - Sidekiq.redis(&:flushdb) - Sidekiq.redis { |conn| expect(conn.llen('queue:customqueue')).to eq(0) } - - 10.times do - Sidekiq::Client.push( - 'class' => MyUniqueJobWithFilterProc, - 'queue' => 'customqueue', - 'args' => [1, type: 'value', some: 'not used'], - ) - end - Sidekiq.redis { |conn| expect(conn.llen('queue:customqueue')).to eq(1) } - end - - it 'does not push duplicate messages when unique_args are filtered with a method' do - 10.times { MyUniqueJobWithFilterMethod.perform_async(1) } - Sidekiq.redis do |conn| - expect(conn.llen('queue:customqueue')).to eq(1) - end - Sidekiq.redis(&:flushdb) - Sidekiq.redis { |conn| expect(conn.llen('queue:customqueue')).to eq(0) } - 10.times do - Sidekiq::Client.push( - 'class' => MyUniqueJobWithFilterMethod, - 'queue' => 'customqueue', - 'args' => [1, type: 'value', some: 'not used'], - ) - end - Sidekiq.redis { |conn| expect(conn.llen('queue:customqueue')).to eq(1) } - end - - it 'does push duplicate messages to different queues' do - Sidekiq::Client.push('class' => MyUniqueJob, 'queue' => 'customqueue', 'args' => [1, 2]) - Sidekiq::Client.push('class' => MyUniqueJob, 'queue' => 'customqueue2', 'args' => [1, 2]) - Sidekiq.redis do |conn| - expect(conn.llen('queue:customqueue')).to eq 1 - expect(conn.llen('queue:customqueue2')).to eq 1 - end - end - - it 'does not queue duplicates when when calling delay' do - 10.times { PlainClass.delay(unique: :until_executed, queue: 'customqueue').run(1) } - Sidekiq.redis do |conn| - expect(conn.llen('queue:customqueue')).to eq(1) - end - end - - it 'does not schedule duplicates when calling perform_in' do - 10.times { MyUniqueJob.perform_in(60, [1, 2]) } - Sidekiq.redis do |conn| - expect(conn.zcount('schedule', -1, Time.now.to_f + 2 * 60)) - .to eq(1) - end - end - - it 'enqueues previously scheduled job' do - jid = WhileExecutingJob.perform_in(60 * 60, 1, 2) - item = { 'class' => WhileExecutingJob, 'queue' => 'customqueue', 'args' => [1, 2], 'jid' => jid } - - # time passes and the job is pulled off the schedule: - Sidekiq::Client.push(item) - - Sidekiq.redis do |conn| - expect(conn.llen('queue:customqueue')).to eq 1 - end - end - - it 'sets an expiration when provided by sidekiq options' do - item = { 'class' => ExpiringJob, 'queue' => 'customqueue', 'args' => [1, 2] } - Sidekiq::Client.push(item) - - Sidekiq.redis do |conn| - expect(conn.llen('queue:customqueue')).to eq(1) - expect(conn.ttl(digest_for(item))) - .to eq(ExpiringJob.get_sidekiq_options['unique_expiration']) - end - end - - it 'does push duplicate messages when not configured for unique only' do - 10.times do - Sidekiq::Client.push('class' => CustomQueueJob, 'queue' => 'customqueue', 'args' => [1, 2]) - end - - Sidekiq.redis do |conn| - expect(conn.llen('queue:customqueue')).to eq(10) - end - end - - describe 'when unique_args is defined' do - it 'does not push duplicate messages based on args filter method' do - expect(CustomQueueJobWithFilterMethod).to respond_to(:args_filter) - expect(CustomQueueJobWithFilterMethod.get_sidekiq_options['unique_args']).to eq :args_filter - - (0..10).each do |i| - Sidekiq::Client.push( - 'class' => CustomQueueJobWithFilterMethod, - 'queue' => 'customqueue', - 'args' => [1, i], - ) - end - - Sidekiq.redis do |conn| - expect(conn.llen('queue:customqueue')).to eq(1) - end - end - - it 'does not push duplicate messages based on args filter proc' do - expect(CustomQueueJobWithFilterProc.get_sidekiq_options['unique_args']).to be_a(Proc) - - 100.times do - Sidekiq::Client.push( - 'class' => CustomQueueJobWithFilterProc, - 'queue' => 'customqueue', - 'args' => [1, { random: rand, name: 'foobar' }], - ) - end - - Sidekiq.redis do |conn| - expect(conn.llen('queue:customqueue')).to eq(1) - end - end - - describe 'when unique_on_all_queues is set' do - it 'does not push duplicate messages on different queues' do - item = { 'class' => UniqueOnAllQueuesJob, 'args' => [1, 2] } - Sidekiq::Client.push(item.merge('queue' => 'customqueue')) - Sidekiq::Client.push(item.merge('queue' => 'customqueue2')) - Sidekiq.redis do |conn| - expect(conn.llen('queue:customqueue')).to eq(1) - expect(conn.llen('queue:customqueue2')).to eq(0) - end - end - end - - describe 'when unique_on_all_queues is set' do - it 'does not push duplicate messages for other workers' do - item_one = { - 'queue' => 'customqueue1', - 'class' => UniqueAcrossWorkersJob, - 'unique_across_workers' => true, - 'args' => [1, 2], - } - - item_two = { - 'queue' => 'customqueue1', - 'class' => MyUniqueJob, - 'unique_across_workers' => true, - 'args' => [1, 2], - } - - Sidekiq::Client.push(item_one) - Sidekiq::Client.push(item_two) - Sidekiq.redis do |conn| - expect(conn.llen('queue:customqueue1')).to eq(1) - end - end - end - end - - # TODO: If anyone know of a better way to check that the expiration for scheduled - # jobs are set around the same time as the scheduled job itself feel free to improve. - it 'expires the digest when a scheduled job is scheduled at' do - expected_expires_at = - (Time.at(Time.now.to_i + 15 * 60) - Time.now.utc) + - SidekiqUniqueJobs.config.default_queue_lock_expiration - jid = MyUniqueJob.perform_in(expected_expires_at, 'mike') - item = { 'class' => MyUniqueJob, - 'queue' => 'customqueue', - 'args' => ['mike'], - 'at' => expected_expires_at } - digest = digest_for(item.merge('jid' => jid)) - Sidekiq.redis do |conn| - expect(conn.ttl(digest)).to eq(9_899) - end - end - - it 'logs duplicate payload when config turned on' do - expect(Sidekiq.logger).to receive(:warn).with(/^payload is not unique/) - UntilExecutedJob.sidekiq_options log_duplicate_payload: true - 2.times do - Sidekiq::Client.push('class' => UntilExecutedJob, 'queue' => 'customqueue', 'args' => [1, 2]) - end - Sidekiq.redis do |conn| - expect(conn.llen('queue:customqueue')).to eq 1 - end - UntilExecutedJob.sidekiq_options log_duplicate_payload: true - end - - it 'does not log duplicate payload when config turned off' do - expect(SidekiqUniqueJobs.logger).to_not receive(:warn).with(/^payload is not unique/) - - UntilExecutedJob.sidekiq_options log_duplicate_payload: false - - 2.times do - Sidekiq::Client.push('class' => UntilExecutedJob, 'queue' => 'customqueue', 'args' => [1, 2]) - end - Sidekiq.redis do |conn| - expect(conn.llen('queue:customqueue')).to eq 1 - end - UntilExecutedJob.sidekiq_options log_duplicate_payload: true - end - end - - describe '#call' do - let(:worker_class) { SimpleWorker } - let(:item) do - { 'class' => SimpleWorker, - 'queue' => 'default', - 'args' => [1] } - end - let(:queue) { 'default' } - context 'when ordinary_or_locked?' do - before do - allow(subject).to receive(:disabled_or_successfully_locked?).and_return(false) - end - - it 'returns nil' do - expect(subject.call(worker_class, item, queue)) - .to eq(nil) - end - end - end -end diff --git a/spec/lib/sidekiq_unique_jobs/lock/until_executed_spec.rb b/spec/lib/sidekiq_unique_jobs/lock/until_executed_spec.rb deleted file mode 100644 index 492557dbe..000000000 --- a/spec/lib/sidekiq_unique_jobs/lock/until_executed_spec.rb +++ /dev/null @@ -1,39 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe SidekiqUniqueJobs::Lock::UntilExecuted do - describe '#execute' do - subject { described_class.new(item) } - let(:item) do - { 'jid' => 'maaaahjid', 'class' => 'UntilExecutedJob', 'unique' => 'until_executed' } - end - let(:empty_callback) { -> {} } - - def execute - subject.execute(empty_callback) - end - - context 'when yield fails with Sidekiq::Shutdown' do - before do - allow(subject).to receive(:after_yield_yield) { raise Sidekiq::Shutdown } - allow(subject).to receive(:unlock).and_return(true) - expect(subject).not_to receive(:unlock) - expect(subject.logger).to receive(:fatal) - expect(empty_callback).not_to receive(:call) - end - - specify { expect { subject.execute(empty_callback) }.to raise_error(Sidekiq::Shutdown) } - end - - context 'when yield fails with other errors' do - before do - allow(subject).to receive(:after_yield_yield) { raise 'Hell' } - expect(subject).to receive(:unlock).and_return(true) - expect(empty_callback).to receive(:call) - end - - specify { expect { subject.execute(empty_callback) }.to raise_error('Hell') } - end - end -end diff --git a/spec/lib/sidekiq_unique_jobs/lock/while_executing_spec.rb b/spec/lib/sidekiq_unique_jobs/lock/while_executing_spec.rb deleted file mode 100644 index 58ff0604b..000000000 --- a/spec/lib/sidekiq_unique_jobs/lock/while_executing_spec.rb +++ /dev/null @@ -1,88 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe SidekiqUniqueJobs::Lock::WhileExecuting do - let(:item) do - { - 'jid' => 'maaaahjid', - 'queue' => 'dupsallowed', - 'class' => 'UntilAndWhileExecuting', - 'unique' => 'until_executed', - 'unique_digest' => 'test_mutex_key', - 'args' => [1], - } - end - - it 'allows only one mutex object to have the lock at a time' do - mutexes = (1..10).map do - described_class.new(item) - end - - x = 0 - mutexes.map do |m| - Thread.new do - m.synchronize do - y = x - sleep 0.1 - x = y + 1 - end - end - end.map(&:join) - - expect(x).to eq(10) - end - - it 'expires the lock on the mutex object after run_lock_expiration if specified' do - mutexes = (1..10).map do - described_class.new(item) - end - - worker = SidekiqUniqueJobs.worker_class_constantize(item[SidekiqUniqueJobs::CLASS_KEY]) - allow(worker).to receive(:get_sidekiq_options) - .and_return('retry' => true, - 'queue' => :dupsallowed, - 'run_lock_expiration' => 0, - 'unique' => :until_and_while_executing) - - start_times = [] - sleep_time = 0.1 - mutexes.each_with_index.map do |m, i| - Thread.new do - m.synchronize do - start_times[i] = Time.now - sleep sleep_time - end - end - end.map(&:join) - - expect(start_times.size).to be 10 - expect(start_times.sort.last - start_times.sort.first).to be < sleep_time * 10 - end - - it 'handles auto cleanup correctly' do - m = described_class.new(item) - - SidekiqUniqueJobs.connection do |conn| - conn.set 'test_mutex_key:run', Time.now.to_i - 1, nx: true - end - - start = Time.now.to_i - m.synchronize do - 'nop' - end - - # no longer than a second - expect(Time.now.to_i).to be <= start + 1 - end - - it 'maintains mutex semantics' do - m = described_class.new(item) - - expect do - m.synchronize do - m.synchronize {} - end - end.to raise_error(ThreadError) - end -end diff --git a/spec/lib/sidekiq_unique_jobs/queue_lock_timeout_calculator_spec.rb b/spec/lib/sidekiq_unique_jobs/queue_lock_timeout_calculator_spec.rb deleted file mode 100644 index 9a8425b1f..000000000 --- a/spec/lib/sidekiq_unique_jobs/queue_lock_timeout_calculator_spec.rb +++ /dev/null @@ -1,49 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe SidekiqUniqueJobs::QueueLockTimeoutCalculator do - shared_context 'generic unscheduled job' do - subject { described_class.new('class' => 'JustAWorker') } - end - - describe 'public api' do - it_behaves_like 'generic unscheduled job' do - it { is_expected.to respond_to(:seconds) } - it { is_expected.to respond_to(:queue_lock_expiration) } - end - end - - describe '.for_item' do - it 'initializes a new calculator' do - expect(described_class).to receive(:new).with('WAT') - described_class.for_item('WAT') - end - end - - describe '#seconds' do - subject { described_class.new(nil) } - - before do - allow(subject).to receive(:time_until_scheduled).and_return(10) - allow(subject).to receive(:queue_lock_expiration).and_return(9) - end - its(:seconds) { is_expected.to eq(19) } - end - - describe '#queue_lock_expiration' do - context 'using default unique_expiration' do - subject { described_class.new(nil) } - before { allow(subject).to receive(:worker_class_queue_lock_expiration).and_return(nil) } - - its(:queue_lock_expiration) { is_expected.to eq(1_800) } - end - - context 'using specified sidekiq option unique_expiration' do - subject { described_class.new(nil) } - before { allow(subject).to receive(:worker_class_queue_lock_expiration).and_return(9) } - - its(:queue_lock_expiration) { is_expected.to eq(9) } - end - end -end diff --git a/spec/lib/sidekiq_unique_jobs/run_lock_timeout_calculator_spec.rb b/spec/lib/sidekiq_unique_jobs/run_lock_timeout_calculator_spec.rb deleted file mode 100644 index 3574f99b2..000000000 --- a/spec/lib/sidekiq_unique_jobs/run_lock_timeout_calculator_spec.rb +++ /dev/null @@ -1,44 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe SidekiqUniqueJobs::RunLockTimeoutCalculator do - subject { calculator } - - let(:calculator) { described_class.new(args) } - let(:args) { { 'class' => 'JustAWorker' } } - - it { is_expected.to respond_to(:seconds) } - - describe '.for_item' do - subject { described_class.for_item('WAT') } - - it 'initializes a new calculator' do - expect(described_class).to receive(:new).with('WAT') - subject - end - end - - describe '#seconds' do - subject { calculator.seconds } - - before do - allow(calculator).to receive(:worker_class_run_lock_expiration) - .and_return(expiration) - end - - context 'when worker_class_run_lock_expiration is configured' do - let(:args) { nil } - let(:expiration) { 9 } - - it { is_expected.to eq(9) } - end - - context 'when worker_class_run_lock_expiration is not configured' do - let(:args) { nil } - let(:expiration) { nil } - - it { is_expected.to eq(60) } - end - end -end diff --git a/spec/lib/sidekiq_unique_jobs/script_mock_spec.rb b/spec/lib/sidekiq_unique_jobs/script_mock_spec.rb deleted file mode 100644 index 79ad191e1..000000000 --- a/spec/lib/sidekiq_unique_jobs/script_mock_spec.rb +++ /dev/null @@ -1,109 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -begin - require 'mock_redis' - MOCK_REDIS ||= MockRedis.new -rescue LoadError # rubocop:disable Lint/HandleExceptions - # This is a known issue (we only run this spec for ruby 2.4.1) -end - -RSpec.describe SidekiqUniqueJobs::ScriptMock, ruby_ver: '>= 2.4.1' do - before do - SidekiqUniqueJobs.configure do |config| - config.redis_test_mode = :mock - end - Sidekiq::Worker.clear_all - - keys = MOCK_REDIS.keys - if keys.respond_to?(:each) - keys.each do |key| - MOCK_REDIS.del(key) - end - else - MOCK_REDIS.del(keys) - end - - allow(Sidekiq).to receive(:redis).and_yield(MOCK_REDIS) - end - - after do - SidekiqUniqueJobs.configure do |config| - config.redis_test_mode = :redis - end - end - - shared_context 'shared SciptMock setup' do - let(:redis_pool) { nil } - let(:unique_key) { 'uniquejobs:unique' } - let(:jid) { 'fuckit' } - let(:another_jid) { 'anotherjid' } - let(:seconds) { 1 } - end - - def acquire_lock(custom_jid = nil) - described_class.acquire_lock( - redis_pool, - keys: [unique_key], - argv: [custom_jid || jid, seconds], - ) - end - - describe '.acquire_lock' do - subject { acquire_lock } - - include_context 'shared SciptMock setup' - - context 'when job is unique' do - it do - expect(acquire_lock).to eq(1) - expect(SidekiqUniqueJobs) - .to have_key(unique_key) - .for_seconds(1) - .with_value('fuckit') - sleep 1 - expect(acquire_lock).to eq(1) - end - end - - context 'when job is not unique' do - let(:seconds) { 10 } - before { expect(acquire_lock(another_jid)).to eq(1) } - - it { is_expected.to eq(0) } - end - end - - def release_lock(custom_jid = nil) - described_class.release_lock( - redis_pool, - keys: [unique_key], - argv: [custom_jid || jid, seconds], - ) - end - - describe '.release_lock' do - subject { release_lock } - - include_context 'shared SciptMock setup' - - context 'when job is locked by another jid' do - before { expect(acquire_lock(another_jid)).to eq(1) } - after { expect(release_lock(another_jid)).to eq(1) } - - it { is_expected.to eq(0) } - end - - context 'when job is not locked at all' do - specify { expect(release_lock).to eq(-1) } - end - - context 'when job is locked by the same jid' do - let(:seconds) { 10 } - before { expect(acquire_lock).to eq(1) } - - it { is_expected.to eq(1) } - end - end -end diff --git a/spec/lib/sidekiq_unique_jobs/scripts/acquire_lock_spec.rb b/spec/lib/sidekiq_unique_jobs/scripts/acquire_lock_spec.rb deleted file mode 100644 index 9ddd70c84..000000000 --- a/spec/lib/sidekiq_unique_jobs/scripts/acquire_lock_spec.rb +++ /dev/null @@ -1,52 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe SidekiqUniqueJobs::Scripts::AcquireLock do - let(:redis_pool) { nil } - let(:jid) { 'abcdefab' } - let(:unique_key) { 'uniquejobs:123asdasd2134' } - let(:max_lock_time) { 1 } - - describe '.execute' do - subject { instance_double(described_class) } - - it 'delegates to instance' do - expect(described_class).to receive(:new) - .with(redis_pool, unique_key, jid, max_lock_time) - .and_return(subject) - expect(subject).to receive(:execute).and_return(true) - - described_class.execute(redis_pool, unique_key, jid, max_lock_time) - end - end - - describe '#execute' do - context 'when job is unique' do - def execute(myjid = jid, key = unique_key, max_lock = max_lock_time) - described_class.execute( - redis_pool, - key, - myjid, - max_lock, - ) - end - - specify { expect(execute(jid, unique_key, max_lock_time)).to eq(true) } - specify do - expect(execute(jid, unique_key, max_lock_time)).to eq(true) - expect(SidekiqUniqueJobs) - .to have_key(unique_key) - .for_seconds(max_lock_time) - .with_value('abcdefab') - sleep 0.5 - expect(execute).to eq(true) - end - - context 'when a unique_key exists for another jid' do - before { expect(execute(jid, unique_key, 10)).to eq(true) } - specify { expect(execute('anotherjid', unique_key, 5)).to eq(false) } - end - end - end -end diff --git a/spec/lib/sidekiq_unique_jobs/scripts/release_lock_spec.rb b/spec/lib/sidekiq_unique_jobs/scripts/release_lock_spec.rb deleted file mode 100644 index 0bccb5514..000000000 --- a/spec/lib/sidekiq_unique_jobs/scripts/release_lock_spec.rb +++ /dev/null @@ -1,43 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe SidekiqUniqueJobs::Scripts::ReleaseLock do - let(:redis_pool) { nil } - let(:jid) { 'abcdefab' } - let(:unique_key) { 'uniquejobs:123asdasd2134' } - let(:max_lock_time) { 1 } - - describe '.execute' do - subject { instance_double(described_class) } - - it 'delegates to instance' do - expect(described_class).to receive(:new) - .with(redis_pool, unique_key, jid) - .and_return(subject) - expect(subject).to receive(:execute).and_return(true) - - described_class.execute(redis_pool, unique_key, jid) - end - end - - describe '#execute' do - context 'when exists' do - subject { described_class.execute(redis_pool, unique_key, jid) } - - before do - SidekiqUniqueJobs::Scripts::AcquireLock.execute(redis_pool, unique_key, jid, max_lock_time) - end - - specify do - expect(SidekiqUniqueJobs) - .to have_key(unique_key) - .for_seconds(max_lock_time) - .with_value(jid) - - expect(subject).to eq(true) - expect(SidekiqUniqueJobs).not_to have_key(unique_key) - end - end - end -end diff --git a/spec/lib/sidekiq_unique_jobs/server/middleware_spec.rb b/spec/lib/sidekiq_unique_jobs/server/middleware_spec.rb deleted file mode 100644 index cfcecc1c5..000000000 --- a/spec/lib/sidekiq_unique_jobs/server/middleware_spec.rb +++ /dev/null @@ -1,80 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' -require 'sidekiq/api' -require 'sidekiq/cli' -require 'sidekiq/worker' -require 'sidekiq_unique_jobs/server/middleware' - -RSpec.describe SidekiqUniqueJobs::Server::Middleware do - QUEUE ||= 'working' - - def digest_for(item) - SidekiqUniqueJobs::UniqueArgs.digest(item) - end - - describe '#call' do - context 'when unique is disabled' do - it 'does not use locking' do - allow(subject).to receive(:unique_enabled?).and_return(false) - expect(subject).not_to receive(:lock) - args = [WhileExecutingJob, { 'class' => 'WhileExecutingJob' }, 'working', nil] - subject.call(*args) {} - end - end - - context 'when unique is enabled' do - it 'executes the lock' do - allow(subject).to receive(:unique_enabled?).and_return(true) - lock = instance_spy(SidekiqUniqueJobs::Lock::WhileExecuting) - expect(lock).to receive(:send).with(:execute, instance_of(Proc)).and_yield - expect(subject).to receive(:lock).and_return(lock) - - args = [WhileExecutingJob, { 'class' => 'WhileExecutingJob' }, 'working', nil] - subject.call(*args) {} - end - end - - describe '#unlock' do - it 'does not unlock mutexes it does not own' do - jid = UntilExecutedJob.perform_async - item = Sidekiq::Queue.new(QUEUE).find_job(jid).item - Sidekiq.redis do |conn| - conn.set(digest_for(item), 'NOT_DELETED') - end - - subject.call(UntilExecutedJob.new, item, QUEUE) do - Sidekiq.redis do |conn| - expect(conn.get(digest_for(item))).to eq('NOT_DELETED') - end - end - end - end - - describe ':before_yield' do - it 'removes the lock before yielding to the worker' do - jid = UntilExecutingJob.perform_async - item = Sidekiq::Queue.new(QUEUE).find_job(jid).item - worker = UntilExecutingJob.new - subject.call(worker, item, QUEUE) do - Sidekiq.redis do |conn| - expect(conn.ttl(digest_for(item))).to eq(-2) # key does not exist - end - end - end - end - - describe ':after_yield' do - it 'removes the lock after yielding to the worker' do - jid = UntilExecutedJob.perform_async - item = Sidekiq::Queue.new(QUEUE).find_job(jid).item - - subject.call('UntilExecutedJob', item, QUEUE) do - Sidekiq.redis do |conn| - expect(conn.get(digest_for(item))).to eq jid - end - end - end - end - end -end diff --git a/spec/lib/sidekiq_unique_jobs/sidekiq_testing_enabled_spec.rb b/spec/lib/sidekiq_unique_jobs/sidekiq_testing_enabled_spec.rb deleted file mode 100644 index bb8214659..000000000 --- a/spec/lib/sidekiq_unique_jobs/sidekiq_testing_enabled_spec.rb +++ /dev/null @@ -1,166 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' -require 'sidekiq/worker' -require 'sidekiq-unique-jobs' -require 'sidekiq/scheduled' - -RSpec.describe 'When Sidekiq::Testing is enabled' do - describe 'when set to :fake!', sidekiq: :fake do - context 'with unique worker' do - it 'does not push duplicate messages' do - param = 'work' - expect(UntilExecutedJob.jobs.size).to eq(0) - expect(UntilExecutedJob.perform_async(param)).to_not be_nil - expect(UntilExecutedJob.jobs.size).to eq(1) - expect(UntilExecutedJob.perform_async(param)).to be_nil - expect(UntilExecutedJob.jobs.size).to eq(1) - end - - it 'unlocks jobs after draining a worker' do - param = 'work' - param2 = 'more work' - - expect(UntilExecutedJob.jobs.size).to eq(0) - UntilExecutedJob.perform_async(param) - UntilExecutedJob.perform_async(param2) - expect(UntilExecutedJob.jobs.size).to eq(2) - UntilExecutedJob.drain - expect(UntilExecutedJob.jobs.size).to eq(0) - UntilExecutedJob.perform_async(param) - UntilExecutedJob.perform_async(param2) - expect(UntilExecutedJob.jobs.size).to eq(2) - end - - it 'unlocks a single job when calling perform_one' do - param = 'work' - param2 = 'more work' - expect(UntilExecutedJob.jobs.size).to eq(0) - UntilExecutedJob.perform_async(param) - UntilExecutedJob.perform_async(param2) - expect(UntilExecutedJob.jobs.size).to eq(2) - UntilExecutedJob.perform_one - expect(UntilExecutedJob.jobs.size).to eq(1) - UntilExecutedJob.perform_async(param2) - expect(UntilExecutedJob.jobs.size).to eq(1) - UntilExecutedJob.perform_async(param) - expect(UntilExecutedJob.jobs.size).to eq(2) - end - - it 'unlocks jobs cleared from a single worker' do - param = 'work' - param2 = 'more work' - expect(UntilExecutedJob.jobs.size).to eq(0) - expect(AnotherUniqueJob.jobs.size).to eq(0) - UntilExecutedJob.perform_async(param) - UntilExecutedJob.perform_async(param2) - AnotherUniqueJob.perform_async(param) - expect(UntilExecutedJob.jobs.size).to eq(2) - expect(AnotherUniqueJob.jobs.size).to eq(1) - UntilExecutedJob.clear - expect(UntilExecutedJob.jobs.size).to eq(0) - expect(AnotherUniqueJob.jobs.size).to eq(1) - UntilExecutedJob.perform_async(param) - UntilExecutedJob.perform_async(param2) - AnotherUniqueJob.perform_async(param) - expect(UntilExecutedJob.jobs.size).to eq(2) - expect(AnotherUniqueJob.jobs.size).to eq(1) - end - - it 'handles clearing an empty worker queue' do - param = 'work' - UntilExecutedJob.perform_async(param) - UntilExecutedJob.drain - expect(UntilExecutedJob.jobs.size).to eq(0) - expect { UntilExecutedJob.clear }.not_to raise_error - end - - it 'unlocks jobs when all workers are cleared' do - param = 'work' - expect(UntilExecutedJob.jobs.size).to eq(0) - expect(AnotherUniqueJob.jobs.size).to eq(0) - UntilExecutedJob.perform_async(param) - AnotherUniqueJob.perform_async(param) - expect(UntilExecutedJob.jobs.size).to eq(1) - expect(AnotherUniqueJob.jobs.size).to eq(1) - Sidekiq::Worker.clear_all - expect(UntilExecutedJob.jobs.size).to eq(0) - expect(AnotherUniqueJob.jobs.size).to eq(0) - UntilExecutedJob.perform_async(param) - expect(UntilExecutedJob.jobs.size).to eq(1) - AnotherUniqueJob.perform_async(param) - expect(AnotherUniqueJob.jobs.size).to eq(1) - end - - it 'handles clearing all workers when there are no jobs' do - param = 'work' - UntilExecutedJob.perform_async(param) - AnotherUniqueJob.perform_async(param) - Sidekiq::Worker.clear_all - expect(UntilExecutedJob.jobs.size).to eq(0) - expect(AnotherUniqueJob.jobs.size).to eq(0) - expect { Sidekiq::Worker.jobs.size }.not_to raise_error - end - - it 'adds the unique_digest to the message' do - param = 'hash' - item = { 'class' => 'UntilExecutedJob', 'queue' => 'working', 'args' => [param] } - hash = SidekiqUniqueJobs::UniqueArgs.digest(item) - expect(UntilExecutedJob.perform_async(param)).to_not be_nil - expect(UntilExecutedJob.jobs.size).to eq(1) - expect(UntilExecutedJob.jobs.last['unique_digest']).to eq(hash) - end - end - - context 'with non-unique worker' do - it 'pushes duplicates messages' do - param = 'work' - expect(MyJob.jobs.size).to eq(0) - MyJob.perform_async(param) - expect(MyJob.jobs.size).to eq(1) - MyJob.perform_async(param) - expect(MyJob.jobs.size).to eq(2) - end - end - end - - describe 'when set to :inline!', sidekiq: :inline do - it 'once the job is completed allows to run another one' do - expect(TestClass).to receive(:run).with('test').twice - InlineWorker.perform_async('test') - InlineWorker.perform_async('test') - end - - it 'if the unique is kept forever it does not allows to run the job again' do - expect(TestClass).to receive(:run).with('args').once - - UntilGlobalTimeoutJob.perform_async('args') - UntilGlobalTimeoutJob.perform_async('args') - end - - describe 'when a job is set to run once in 10 minutes' do - context 'when spammed' do - it 'only allows 1 call per 10 minutes' do - expect(TestClass).to receive(:run).with(1).once - 100.times do - UntilTimeoutJob.perform_async(1) - end - end - end - - context 'with different arguments' do - it 'only allows 1 call per 10 minutes' do - expect(TestClass).to receive(:run).with(9).once - 2.times do - UntilTimeoutJob.perform_async(9) - end - - expect(TestClass).to receive(:run).with(2).once - 2.times do - UntilTimeoutJob.perform_async(2) - end - end - end - end - end -end diff --git a/spec/lib/sidekiq_unique_jobs/unlockable_spec.rb b/spec/lib/sidekiq_unique_jobs/unlockable_spec.rb deleted file mode 100644 index 05511bf3b..000000000 --- a/spec/lib/sidekiq_unique_jobs/unlockable_spec.rb +++ /dev/null @@ -1,58 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe SidekiqUniqueJobs::Unlockable do - def item_with_digest - SidekiqUniqueJobs::UniqueArgs.digest(item) - item - end - let(:item) do - { 'class' => MyUniqueJob, - 'queue' => 'customqueue', - 'args' => [1, 2] } - end - - let(:unique_digest) { item_with_digest[SidekiqUniqueJobs::UNIQUE_DIGEST_KEY] } - - describe '.unlock' do - subject { described_class.unlock(item_with_digest) } - - context 'when item is missing unique digest key' do - subject { described_class.unlock(item) } - it { is_expected.to eq(nil) } - specify do - expect(described_class).not_to receive(:unlock_by_key) - end - end - - specify do - jid = Sidekiq::Client.push(item_with_digest) - expect(described_class).to receive(:unlock_by_key).with(unique_digest, jid) - subject - end - end - - describe '.unlock_by_key' do - before do - end - - specify do - expect(SidekiqUniqueJobs::Util.keys.count).to eq(0) - jid = Sidekiq::Client.push(item_with_digest) - - expect(SidekiqUniqueJobs::Util.keys.count).to eq(1) - expect(SidekiqUniqueJobs::Util.keys).to match_array([unique_digest]) - expect(SidekiqUniqueJobs::Util.unique_key(jid)).to eq(unique_digest) - - described_class.unlock_by_key( - unique_digest, - jid, - ) - - expect(SidekiqUniqueJobs::Util.keys.count).to eq(0) - expect(SidekiqUniqueJobs::Util.keys).not_to match_array([unique_digest]) - expect(SidekiqUniqueJobs::Util.unique_key(jid)).to eq(nil) - end - end -end diff --git a/spec/lib/sidekiq_unique_jobs/util_spec.rb b/spec/lib/sidekiq_unique_jobs/util_spec.rb deleted file mode 100644 index 43a5657e3..000000000 --- a/spec/lib/sidekiq_unique_jobs/util_spec.rb +++ /dev/null @@ -1,84 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe SidekiqUniqueJobs::Util do - let(:item) do - { - 'class' => 'MyUniqueJob', - 'args' => [[1, 2]], - 'at' => 1_492_341_850.358196, - 'retry' => true, - 'queue' => 'customqueue', - 'unique' => :until_executed, - 'unique_expiration' => 7200, - 'retry_count' => 10, - 'jid' => jid, - 'created_at' => 1_492_341_790.358217, - } - end - let(:unique_args) { SidekiqUniqueJobs::UniqueArgs.new(item) } - let(:unique_key) { unique_args.unique_digest } - let(:jid) { 'e3049b05b0bd9c809182bbe0' } - - def acquire_lock - SidekiqUniqueJobs::Scripts::AcquireLock.execute(nil, unique_key, jid, 1) - end - - describe '.keys' do - end - - describe '.del' do - before do - acquire_lock - end - - it 'deletes the keys by pattern' do - expect(described_class.del(described_class::SCAN_PATTERN, 100, false)).to eq(1) - end - - it 'deletes the keys by distinct key' do - expect(described_class.del(unique_key, 100, false)).to eq(1) - end - end - - describe '.expire' do - before do - acquire_lock - end - - it 'does some shit' do - sleep 1 - expected = { jid => unique_key } - expect(described_class.unique_hash).to match(expected) - removed_keys = described_class.expire - expect(removed_keys).to match(expected) - expect(described_class.unique_hash).not_to match(expected) - expect(described_class.keys('*')).not_to include(unique_key) - end - end - - describe '.prefix' do - subject { described_class.send(:prefix, key) } - - let(:key) { 'key' } - - context 'when prefix is configured' do - before { allow(SidekiqUniqueJobs.config).to receive(:unique_prefix).and_return('test-uniqueness') } - - it { is_expected.to eq('test-uniqueness:key') } - - context 'when key is already prefixed' do - let(:key) { 'test-uniqueness:key' } - - it { is_expected.to eq('test-uniqueness:key') } - end - end - - context 'when .unique_prefix is nil?' do - before { allow(SidekiqUniqueJobs.config).to receive(:unique_prefix).and_return(nil) } - - it { is_expected.to eq('key') } - end - end -end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index e76baea1d..32c556df5 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -1,11 +1,12 @@ # frozen_string_literal: true VERSION_REGEX = /(?[<>=]+)?\s?(?(\d+.?)+)/m -if RUBY_ENGINE == 'ruby' && RUBY_VERSION >= '2.4.0' +if RUBY_ENGINE == 'ruby' && RUBY_VERSION >= '2.4.1' require 'simplecov' begin - require 'pry-byebug' + require 'pry' + require 'byebug' rescue LoadError puts 'Pry unavailable' end @@ -13,6 +14,7 @@ require 'rspec' require 'rspec/its' +require 'awesome_print' require 'sidekiq' require 'sidekiq/util' @@ -22,23 +24,14 @@ require 'sidekiq/simulator' Sidekiq::Testing.disable! +Sidekiq.logger = Logger.new('/dev/null') SidekiqUniqueJobs.logger.level = Object.const_get("Logger::#{ENV.fetch('LOGLEVEL') { 'error' }.upcase}") require 'sidekiq/redis_connection' -REDIS_URL ||= ENV['REDIS_URL'] || 'redis://localhost/15' -REDIS_NAMESPACE ||= 'unique-test' -REDIS_OPTIONS ||= { url: REDIS_URL } # rubocop:disable MutableConstant -REDIS_OPTIONS[:namespace] = REDIS_NAMESPACE if defined?(Redis::Namespace) -REDIS ||= Sidekiq::RedisConnection.create(REDIS_OPTIONS) - -Sidekiq.configure_client do |config| - config.redis = REDIS_OPTIONS -end - Dir[File.join(File.dirname(__FILE__), 'support', '**', '*.rb')].each { |f| require f } -RSpec.configure do |config| # rubocop:disable BlockLength +RSpec.configure do |config| config.define_derived_metadata do |meta| meta[:aggregate_failures] = true end @@ -48,6 +41,7 @@ config.mock_with :rspec do |mocks| mocks.verify_partial_doubles = true end + config.example_status_persistence_file_path = 'spec/examples.txt' config.filter_run :focus unless ENV['CI'] config.run_all_when_everything_filtered = true config.disable_monkey_patching! @@ -55,30 +49,6 @@ config.default_formatter = 'doc' if config.files_to_run.one? config.order = :random Kernel.srand config.seed - - config.before(:each) do - SidekiqUniqueJobs.configure do |unique_config| - unique_config.redis_test_mode = :redis - end - Sidekiq.redis = REDIS - Sidekiq.redis(&:flushdb) - Sidekiq::Worker.clear_all - Sidekiq::Queues.clear_all - - if Sidekiq::Testing.respond_to?(:server_middleware) - Sidekiq::Testing.server_middleware do |chain| - chain.add SidekiqUniqueJobs::Server::Middleware - end - end - enable_delay = defined?(Sidekiq::Extensions) && Sidekiq::Extensions.respond_to?(:enable_delay!) - Sidekiq::Extensions.enable_delay! if enable_delay - end - - config.after(:each) do - Sidekiq.redis(&:flushdb) - respond_to_middleware = defined?(Sidekiq::Testing) && Sidekiq::Testing.respond_to?(:server_middleware) - Sidekiq::Testing.server_middleware(&:clear) if respond_to_middleware - end end Dir[File.join(File.dirname(__FILE__), 'jobs', '**', '*.rb')].each { |f| require f } diff --git a/spec/support/matchers/redis_matchers.rb b/spec/support/matchers/redis_matchers.rb index 78fc5ce31..cb355a238 100644 --- a/spec/support/matchers/redis_matchers.rb +++ b/spec/support/matchers/redis_matchers.rb @@ -5,8 +5,9 @@ RSpec::Matchers.define :have_key do |unique_key| Sidekiq.redis do |conn| match do |_unique_jobs| - @value = conn.get(unique_key) - @ttl = conn.ttl(unique_key) + @exists_key = "#{unique_key}:EXISTS" + @value = conn.get(@exists_key) + @ttl = conn.ttl(@exists_key) @value && with_value && for_seconds end @@ -22,9 +23,9 @@ end failure_message do |_actual| - msg = "expected Redis to have key #{unique_key}" - msg << " with value #{@expected_value} was (#{@value})" if @expected_value - msg << " with value #{@expected_ttl} was (#{@ttl})" if @expected_ttl + msg = "expected Redis to have key #{@exists_key}" + msg += " with value #{@expected_value} was (#{@value})" if @expected_value + msg += " with value #{@expected_ttl} was (#{@ttl})" if @expected_ttl msg end end diff --git a/spec/support/sidekiq_meta.rb b/spec/support/sidekiq_meta.rb index aad35dbbd..8f20ab24b 100644 --- a/spec/support/sidekiq_meta.rb +++ b/spec/support/sidekiq_meta.rb @@ -1,12 +1,49 @@ # frozen_string_literal: true -RSpec.configure do |config| +RSpec.configure do |config| # rubocop:disable Metrics/BlockLength + config.before(:each, redis: :mock_redis) do + require 'mock_redis' + mock_redis = MockRedis.new + SidekiqUniqueJobs.configure do |unique| + unique.redis_test_mode = :mock + end + allow(SidekiqUniqueJobs).to receive(:mocked?).and_return(true) + allow(SidekiqUniqueJobs).to receive(:redis_version).and_return('0.0') + Sidekiq::Worker.clear_all + + allow(Sidekiq).to receive(:redis).and_yield(mock_redis) + end + + config.before(:each, redis: :redis) do |example| + redis_db = example.metadata.fetch(:redis_db) { 0 } + redis_url = "redis://localhost/#{redis_db}" + redis_options = { url: redis_url } + redis = Sidekiq::RedisConnection.create(redis_options) + + Sidekiq.configure_client do |sidekiq_config| + sidekiq_config.redis = redis_options + end + + SidekiqUniqueJobs.configure do |unique_config| + unique_config.redis_test_mode = :redis + end + + Sidekiq.redis = redis + Sidekiq.redis(&:flushdb) + end + config.before(:each) do |example| - SidekiqUniqueJobs.configure do |conn| - conn.redis_test_mode = :redis + Sidekiq::Worker.clear_all + Sidekiq::Queues.clear_all + + Sidekiq::Testing.server_middleware do |chain| + chain.add SidekiqUniqueJobs::Server::Middleware end - if (sidekiq = example.metadata[:sidekiq]) + enable_delay = defined?(Sidekiq::Extensions) && Sidekiq::Extensions.respond_to?(:enable_delay!) + Sidekiq::Extensions.enable_delay! if enable_delay + + if (sidekiq = example.metadata.fetch(:sidekiq) { :disable }) sidekiq = :fake if sidekiq == true Sidekiq::Testing.send("#{sidekiq}!") end @@ -26,7 +63,16 @@ end end - config.after(:each) do |example| + config.after(:each, redis: :mock_redis) do + SidekiqUniqueJobs.configure do |unique| + unique.redis_test_mode = :redis + end + end + + config.after(:each, redis: :redis) do |example| + Sidekiq.redis(&:flushdb) + respond_to_middleware = defined?(Sidekiq::Testing) && Sidekiq::Testing.respond_to?(:server_middleware) + Sidekiq::Testing.server_middleware(&:clear) if respond_to_middleware Sidekiq::Testing.disable! unless example.metadata[:sidekiq].nil? end end diff --git a/spec/lib/sidekiq_unique_jobs/cli_spec.rb b/spec/unit/cli_spec.rb similarity index 62% rename from spec/lib/sidekiq_unique_jobs/cli_spec.rb rename to spec/unit/cli_spec.rb index ae8594682..b412a3afe 100644 --- a/spec/lib/sidekiq_unique_jobs/cli_spec.rb +++ b/spec/unit/cli_spec.rb @@ -2,8 +2,9 @@ require 'spec_helper' require 'thor/runner' +require 'irb' -RSpec.describe SidekiqUniqueJobs::Cli, ruby_ver: '>= 2.4' do +RSpec.describe SidekiqUniqueJobs::Cli, redis: :redis, ruby_ver: '>= 2.4' do describe '#help' do let(:output) { capture(:stdout) { described_class.start(%w[help]) } } let(:banner) do @@ -11,7 +12,6 @@ Commands: jobs console # drop into a console with easy access to helper methods jobs del PATTERN # deletes unique keys from redis by pattern - jobs expire # removes all expired unique keys from the hash in redis jobs help [COMMAND] # Describe available commands or one specific command jobs keys PATTERN # list all unique keys and their expiry time EOS @@ -42,23 +42,6 @@ end end - describe '#help expire' do - let(:output) { capture(:stdout) { described_class.start(%w[help expire]) } } - - let(:banner) do - <<~EOS - Usage: - jobs expire - - removes all expired unique keys from the hash in redis - EOS - end - - it 'displays help about the `expire` command' do - expect(output).to eq(banner) - end - end - describe '#help keys' do let(:output) { capture(:stdout) { described_class.start(%w[help keys]) } } let(:banner) do @@ -80,10 +63,16 @@ end end - let(:pattern) { '*' } + let(:pattern) { '*' } let(:max_lock_time) { 1 } - let(:unique_key) { 'uniquejobs:abcdefab' } - let(:jid) { 'abcdefab' } + let(:unique_key) { 'uniquejobs:abcdefab' } + let(:jid) { 'abcdefab' } + let(:item) do + { + 'jid' => jid, + 'unique_digest' => unique_key, + } + end describe '.keys' do let(:output) { capture(:stdout) { described_class.start(%w[keys * --count 1000]) } } @@ -94,24 +83,16 @@ end context 'when a key exists' do - let(:keys) { ['defghayl', jid, 'poilkij'].sort } before do - keys.each do |jid| - unique_key = "uniquejobs:#{jid}" - SidekiqUniqueJobs::Scripts::AcquireLock.execute(nil, unique_key, jid, 20) - expect(SidekiqUniqueJobs) - .to have_key(unique_key) - .for_seconds(20) - .with_value(jid) - end + SidekiqUniqueJobs::Lock.new(item).lock end after { SidekiqUniqueJobs::Util.del('*', 1000, false) } let(:expected) do <<~EOS - Found 3 keys matching '#{pattern}': - uniquejobs:abcdefab uniquejobs:defghayl uniquejobs:poilkij + Found 2 keys matching '#{pattern}': + uniquejobs:abcdefab:EXISTS uniquejobs:abcdefab:GRABBED EOS end specify do @@ -123,48 +104,22 @@ describe '.del' do let(:expected) do <<~EOS - Deleted 1 keys matching '*' + Deleted 2 keys matching '*' EOS end before do - SidekiqUniqueJobs::Scripts::AcquireLock.execute(nil, unique_key, jid, max_lock_time) - expect(SidekiqUniqueJobs) - .to have_key(unique_key) - .for_seconds(1) - .with_value(jid) + SidekiqUniqueJobs::Lock.new(item).lock end specify do output = capture(:stdout) { described_class.start(%w[del * --no-dry-run --count 1000]) } expect(output).to eq(expected) - expect(SidekiqUniqueJobs).not_to have_key(unique_key) - end - end - - describe '.expire' do - before do - SidekiqUniqueJobs::Scripts::AcquireLock.execute(nil, unique_key, jid, max_lock_time) - expect(SidekiqUniqueJobs) - .to have_key(unique_key) - .for_seconds(1) - .with_value(jid) - end - let(:expected) do - <<~EOS - Removed 1 left overs from redis. - uniquejobs:abcdefab - EOS - end - - specify do - sleep 1 - output = capture(:stdout) { described_class.start(%w[expire]) } - expect(output).to eq(expected) + expect(SidekiqUniqueJobs::Util.keys).to eq([]) end end - describe '.console' do + describe '.console', ruby_ver: '>= 2.4.1' do let(:expected) do <<~EOS Use `keys '*', 1000 to display the first 1000 unique keys matching '*' diff --git a/spec/unit/client/middleware_spec.rb b/spec/unit/client/middleware_spec.rb new file mode 100644 index 000000000..d1a45faa7 --- /dev/null +++ b/spec/unit/client/middleware_spec.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'sidekiq/worker' +require 'sidekiq-unique-jobs' +require 'rspec/wait' + +RSpec.describe SidekiqUniqueJobs::Client::Middleware, redis: :redis, redis_db: 2 do + describe '#call' do + subject { middleware.call(worker_class, item, queue) } + + let(:middleware) { described_class.new } + let(:worker_class) { SimpleWorker } + let(:item) do + { 'class' => SimpleWorker, + 'queue' => queue, + 'args' => [1] } + end + let(:queue) { 'default' } + + context 'when ordinary_or_locked?' do + before do + allow(middleware).to receive(:successfully_locked?).and_return(false) + end + + it { is_expected.to be_nil } + end + end +end diff --git a/spec/lib/sidekiq_unique_jobs/config_spec.rb b/spec/unit/config_spec.rb similarity index 100% rename from spec/lib/sidekiq_unique_jobs/config_spec.rb rename to spec/unit/config_spec.rb diff --git a/spec/lib/sidekiq_unique_jobs/core_ext_spec.rb b/spec/unit/core_ext_spec.rb similarity index 100% rename from spec/lib/sidekiq_unique_jobs/core_ext_spec.rb rename to spec/unit/core_ext_spec.rb diff --git a/spec/lib/sidekiq_unique_jobs/lock/until_and_while_executing_spec.rb b/spec/unit/lock/until_and_while_executing_spec.rb similarity index 55% rename from spec/lib/sidekiq_unique_jobs/lock/until_and_while_executing_spec.rb rename to spec/unit/lock/until_and_while_executing_spec.rb index 20866eb9a..f8f256b92 100644 --- a/spec/lib/sidekiq_unique_jobs/lock/until_and_while_executing_spec.rb +++ b/spec/unit/lock/until_and_while_executing_spec.rb @@ -2,36 +2,34 @@ require 'spec_helper' -RSpec.describe SidekiqUniqueJobs::Lock::UntilAndWhileExecuting do +RSpec.describe SidekiqUniqueJobs::Lock::UntilAndWhileExecuting, redis: :redis, redis_db: 3 do + let(:lock) { described_class.new(item) } let(:item) do { 'jid' => 'maaaahjid', 'queue' => 'dupsallowed', - 'class' => 'UntilAndWhileExecuting', + 'class' => 'UntilAndWhileExecutingJob', 'unique' => 'until_executed', 'args' => [1], } end let(:callback) { -> {} } - subject { described_class.new(item) } describe '#execute' do + let(:runtime_lock) { SidekiqUniqueJobs::Lock::WhileExecuting.new(item) } + before do - subject.lock(:client) + Sidekiq.redis(&:flushdb) + allow(lock).to receive(:runtime_lock).and_return(runtime_lock) + lock.lock(:client) end - let(:runtime_lock) { SidekiqUniqueJobs::Lock::WhileExecuting.new(item, nil) } it 'unlocks the unique key before yielding' do - expect(SidekiqUniqueJobs::Lock::WhileExecuting) - .to receive(:new) - .with(item, nil) - .and_return(runtime_lock) - - expect(callback).to receive(:call) + allow(callback).to receive(:call) - subject.execute(callback) do + lock.execute(callback) do Sidekiq.redis do |conn| - expect(conn.keys('uniquejobs:*').size).to eq(1) + expect(conn.keys('uniquejobs:*').size).to eq(0) end 10.times { Sidekiq::Client.push(item) } @@ -40,6 +38,8 @@ expect(conn.keys('uniquejobs:*').size).to eq(2) end end + + expect(callback).to have_received(:call) end end end diff --git a/spec/unit/lock/until_executed_spec.rb b/spec/unit/lock/until_executed_spec.rb new file mode 100644 index 000000000..ee041d0ba --- /dev/null +++ b/spec/unit/lock/until_executed_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe SidekiqUniqueJobs::Lock::UntilExecuted, redis: :redis do + describe '#execute' do + subject { lock.execute(empty_callback) } + + let(:lock) { described_class.new(item) } + let(:empty_callback) { -> {} } + let(:item) do + { + 'jid' => 'maaaahjid', + 'class' => 'UntilExecutedJob', + 'unique' => 'until_executed', + } + end + + context 'when yield fails with Sidekiq::Shutdown' do + before do + allow(lock).to receive(:after_yield_yield) { raise Sidekiq::Shutdown } + allow(lock).to receive(:unlock).and_return(true) + expect(lock).not_to receive(:unlock) + expect(Sidekiq.logger).to receive(:fatal) + .with('the unique_key: uniquejobs:a1e5ccafbc77b234e8f8aaedde3f706e needs to be unlocked manually') + + expect(empty_callback).not_to receive(:call) + end + + specify { expect { subject }.to raise_error(Sidekiq::Shutdown) } + end + + context 'when yield fails with other errors' do + before do + allow(lock).to receive(:after_yield_yield) { raise 'Hell' } + expect(lock).to receive(:unlock).and_return(true) + expect(empty_callback).to receive(:call) + end + + specify { expect { subject }.to raise_error('Hell') } + end + end +end diff --git a/spec/lib/sidekiq_unique_jobs/lock/until_timeout_spec.rb b/spec/unit/lock/until_timeout_spec.rb similarity index 66% rename from spec/lib/sidekiq_unique_jobs/lock/until_timeout_spec.rb rename to spec/unit/lock/until_timeout_spec.rb index ea2aebac7..354b1a5f1 100644 --- a/spec/lib/sidekiq_unique_jobs/lock/until_timeout_spec.rb +++ b/spec/unit/lock/until_timeout_spec.rb @@ -2,8 +2,9 @@ require 'spec_helper' -RSpec.describe SidekiqUniqueJobs::Lock::UntilTimeout do - subject { described_class.new(item) } +RSpec.describe SidekiqUniqueJobs::Lock::UntilTimeout, redis: :redis do + let(:lock) { described_class.new(item) } + let(:item) do { 'jid' => 'maaaahjid', 'class' => 'UntilExecutedJob', @@ -13,14 +14,16 @@ describe '#unlock' do context 'when provided :server' do - it 'returns true' do - expect(subject.unlock(:server)).to eq(true) - end + subject { lock.unlock(:server) } + + it { is_expected.to eq(true) } end context 'when provided with anything else than :server' do + subject { lock.unlock(:client) } + it 'raises a helpful error message' do - expect { subject.unlock(:client) } + expect { subject } .to raise_error(ArgumentError, /client middleware can't unlock uniquejobs:*/) end end diff --git a/spec/unit/lock/while_executing_spec.rb b/spec/unit/lock/while_executing_spec.rb new file mode 100644 index 000000000..9332e3324 --- /dev/null +++ b/spec/unit/lock/while_executing_spec.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe SidekiqUniqueJobs::Lock::WhileExecuting, redis: :redis do +end diff --git a/spec/unit/lock_spec.rb b/spec/unit/lock_spec.rb new file mode 100644 index 000000000..46e56dc3e --- /dev/null +++ b/spec/unit/lock_spec.rb @@ -0,0 +1,250 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'rspec/wait' + +RSpec.shared_context 'a lock setup' do + let(:lock) { described_class.new(lock_item) } + let(:lock_expiration) { nil } + let(:lock_use_local_time) { false } + let(:lock_stale_client_timeout) { nil } + let(:jid) { 'maaaahjid' } + let(:lock_item) do + { + 'jid' => jid, + 'queue' => 'dupsallowed', + 'class' => 'UntilAndWhileExecuting', + 'unique' => 'until_executed', + 'unique_digest' => 'test_mutex_key', + 'args' => [1], + 'lock_expiration' => lock_expiration, + 'use_local_time' => lock_use_local_time, + 'stale_client_timeout' => lock_stale_client_timeout, + } + end + let(:lock_with_different_jid) { described_class.new(lock_item_with_different_jid) } + let(:jid_2) { 'jidmayhem' } + let(:lock_item_with_different_jid) do + lock_item.merge('jid' => jid_2) + end +end + +RSpec.shared_examples_for 'a lock' do + it 'should not exist from the start' do + expect(lock.exists?).to eq(false) + lock.lock + expect(lock.exists?).to eq(true) + end + + it 'should be unlocked from the start' do + expect(lock.locked?).to eq(false) + end + + it 'should lock and unlock' do + lock.lock(1) + expect(lock.locked?).to eq(true) + lock.unlock + expect(lock.locked?).to eq(false) + end + + it 'should not lock twice as a mutex' do + expect(lock.lock(1)).not_to eq(false) + expect(lock.lock(1)).to eq(false) + end + + it 'should execute the given code block' do + code_executed = false + lock.lock(1) do + code_executed = true + end + expect(code_executed).to eq(true) + end + + it 'should pass an exception right through' do + expect do + lock.lock(1) do + raise Exception, 'redis lock exception' + end + end.to raise_error(Exception, 'redis lock exception') + end + + it 'should not leave the lock locked after raising an exception' do + expect do + lock.lock(1) do + raise Exception, 'redis lock exception' + end + end.to raise_error(Exception, 'redis lock exception') + + expect(lock.locked?).to eq(false) + end + + it 'should return the value of the block if block-style locking is used' do + block_value = lock.lock(1) do + 42 + end + expect(block_value).to eq(42) + end + + it 'should disappear without a trace when calling `delete!`' do + original_key_size = SidekiqUniqueJobs.connection { |conn| conn.keys.count } + + lock.exists_or_create! + lock.delete! + + expect(SidekiqUniqueJobs.connection { |conn| conn.keys.count }).to eq(original_key_size) + end + + it 'should not block when the timeout is zero' do + did_we_get_in = false + + lock.lock do + lock.lock(0) do + did_we_get_in = true + end + end + + expect(did_we_get_in).to be false + end + + it 'should be locked when the timeout is zero' do + lock.lock(0) do + expect(lock.locked?).to be true + end + end +end + +RSpec.shared_examples 'a real lock' do + describe 'lock with expiration' do + let(:lock_expiration) { 1 } + + it_behaves_like 'a lock' + + def current_keys + SidekiqUniqueJobs.connection(&:keys) + end + + it 'expires keys' do + Sidekiq.redis(&:flushdb) + lock.exists_or_create! + keys = current_keys + expect(current_keys).not_to include(keys) + end + + it 'expires keys after unlocking' do + Sidekiq.redis(&:flushdb) + lock.lock do + # noop + end + keys = current_keys + sleep 3.0 + expect(current_keys).not_to include(keys) + end + end + + describe 'lock without staleness checking' do + it_behaves_like 'a lock' + + xit 'can have stale locks released by a third process' do + watchdog = described_class.new(lock_item.merge('stale_client_timeout' => 1)) + lock.lock + + sleep 0.3 + watchdog.release_stale_locks! + expect(lock.locked?).to eq(true) + + sleep 0.6 + + watchdog.release_stale_locks! + expect(lock.locked?).to eq(false) + end + end + + describe 'lock with staleness checking' do + let(:lock_stale_client_timeout) { 5 } + + context 'when redis_version is old' do + before do + allow(SidekiqUniqueJobs).to receive(:redis_version).and_return('3.0') + end + + it_behaves_like 'a lock' + + it 'should restore resources of stale clients', redis: :redis do + another_lock_item = lock_item.merge('jid' => 'abcdefab', 'stale_client_timeout' => 1) + hyper_aggressive_lock = described_class.new(another_lock_item) + + expect(hyper_aggressive_lock.lock(1)).not_to eq(false) + expect(hyper_aggressive_lock.lock(1)).to eq(false) + expect(hyper_aggressive_lock.lock(1)).not_to eq(false) + end + end + + context 'when redis_version is new', redis: :redis do + before do + allow(SidekiqUniqueJobs).to receive(:redis_version).and_return('4.0') + end + + it_behaves_like 'a lock' + + it 'should restore resources of stale clients' do + another_lock_item = lock_item.merge('jid' => 'abcdefab', 'stale_client_timeout' => 1) + hyper_aggressive_lock = described_class.new(another_lock_item) + + expect(hyper_aggressive_lock.lock(1)).not_to eq(false) + expect(hyper_aggressive_lock.lock(1)).to eq(false) + expect(hyper_aggressive_lock.lock(1)).not_to eq(false) + end + end + end +end + +RSpec.describe SidekiqUniqueJobs::Lock, 'with Redis', redis: :redis do + include_context 'a lock setup' + it_behaves_like 'a real lock' + + describe 'redis time' do + let(:lock_stale_client_timeout) { 5 } + + before(:all) do + Timecop.freeze(Time.local(1990)) + end + + it 'with time support should return a different time than frozen time' do + expect(lock.send(:current_time)).not_to eq(Time.now) + end + + context 'when use_local_time is true' do + let(:lock_use_local_time) { true } + + it 'with use_local_time should return the same time as frozen time' do + expect(lock.send(:current_time)).to eq(Time.now) + end + end + end +end + +RSpec.describe SidekiqUniqueJobs::Lock, 'with MockRedis', redis: :mock_redis do + require 'mock_redis' + include_context 'a lock setup' + it_behaves_like 'a real lock' + + describe 'redis time' do + subject { lock.send(:current_time) } + + let(:lock_stale_client_timeout) { 5 } + + before(:all) do + Timecop.freeze(Time.local(1990)) + end + + # Since we are never hitting a redis server + # MockRedis uses ruby time for this + it { is_expected.to eq(Time.now) } + + context 'when use_local_time is true' do + let(:lock_use_local_time) { true } + + it { is_expected.to eq(Time.now) } + end + end +end diff --git a/spec/lib/sidekiq_unique_jobs/normalizer_spec.rb b/spec/unit/normalizer_spec.rb similarity index 71% rename from spec/lib/sidekiq_unique_jobs/normalizer_spec.rb rename to spec/unit/normalizer_spec.rb index 3924fd01e..f98bf8dee 100644 --- a/spec/lib/sidekiq_unique_jobs/normalizer_spec.rb +++ b/spec/unit/normalizer_spec.rb @@ -3,21 +3,17 @@ require 'spec_helper' RSpec.describe SidekiqUniqueJobs::Normalizer do - def jsonify(args) - described_class.jsonify(args) - end - describe '.jsonify' do specify do original = [1, :test, [test: :test]] expected = [1, 'test', ['test' => 'test']] - expect(jsonify(original)).to eq(expected) + expect(described_class.jsonify(original)).to eq(expected) end specify do original = [1, :test, [test: [test: :test]]] expected = [1, 'test', ['test' => ['test' => 'test']]] - expect(jsonify(original)).to eq(expected) + expect(described_class.jsonify(original)).to eq(expected) end end end diff --git a/spec/lib/sidekiq_unique_jobs/options_with_fallback_spec.rb b/spec/unit/options_with_fallback_spec.rb similarity index 100% rename from spec/lib/sidekiq_unique_jobs/options_with_fallback_spec.rb rename to spec/unit/options_with_fallback_spec.rb diff --git a/spec/lib/sidekiq_unique_jobs/scripts_spec.rb b/spec/unit/scripts_spec.rb similarity index 93% rename from spec/lib/sidekiq_unique_jobs/scripts_spec.rb rename to spec/unit/scripts_spec.rb index d47f1f09a..d6fb294e1 100644 --- a/spec/lib/sidekiq_unique_jobs/scripts_spec.rb +++ b/spec/unit/scripts_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require 'spec_helper' -RSpec.describe SidekiqUniqueJobs::Scripts do +RSpec.describe SidekiqUniqueJobs::Scripts, redis: :redis do subject { SidekiqUniqueJobs::Scripts } it { is_expected.to respond_to(:call).with(3).arguments } @@ -40,7 +40,7 @@ expect(described_class).to receive(:internal_call).with(script_name, nil, options).once expect { subject }.to raise_error( SidekiqUniqueJobs::ScriptError, - "Problem compiling #{script_name}. Invalid LUA syntax?", + "Problem compiling #{script_name}. Message: Some interesting error", ) end diff --git a/spec/unit/server/middleware_spec.rb b/spec/unit/server/middleware_spec.rb new file mode 100644 index 000000000..0659d733b --- /dev/null +++ b/spec/unit/server/middleware_spec.rb @@ -0,0 +1,95 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'sidekiq/api' +require 'sidekiq/cli' +require 'sidekiq/worker' +require 'sidekiq_unique_jobs/server/middleware' + +RSpec.describe SidekiqUniqueJobs::Server::Middleware, redis: :redis, redis_db: 9 do + let(:middleware) { SidekiqUniqueJobs::Server::Middleware.new } + + let(:queue) { 'working' } + + describe '#call' do + subject { middleware.call(*args) {} } + + let(:args) { [WhileExecutingJob, { 'class' => 'WhileExecutingJob' }, 'working', nil] } + + context 'when unique is disabled' do + before do + allow(middleware).to receive(:unique_disabled?).and_return(true) + end + + it 'does not use locking' do + expect(middleware).not_to receive(:lock) + subject + end + end + + context 'when unique is enabled' do + let(:lock) { instance_spy(SidekiqUniqueJobs::Lock::WhileExecuting) } + + before do + allow(middleware).to receive(:unique_enabled?).and_return(true) + allow(middleware).to receive(:lock).and_return(lock) + end + + it 'executes the lock' do + expect(lock).to receive(:execute).with(instance_of(Proc)).and_yield + subject + end + end + + describe '#unlock' do + it 'does not unlock keys it does not own' do + jid = UntilExecutedJob.perform_async + item = Sidekiq::Queue.new(queue).find_job(jid).item + + lock = SidekiqUniqueJobs::Lock.new(item) + + Sidekiq.redis do |conn| + expect(conn.get(lock.exists_key)).to eq(jid) + conn.set(lock.exists_key, 'NOT_DELETED') + end + + middleware.call(UntilExecutedJob.new, item, queue) do + Sidekiq.redis do |conn| + expect(conn.get(lock.exists_key)).to eq('NOT_DELETED') + end + end + end + end + + describe ':before_yield' do + it 'removes the lock before yielding to the worker' do + jid = UntilExecutingJob.perform_async + item = Sidekiq::Queue.new(queue).find_job(jid).item + worker = UntilExecutingJob.new + + middleware.call(worker, item, queue) do + Sidekiq.redis do |conn| + conn.keys('unique:*').each do |key| + expect(conn.ttl(key)).to eq(-2) # key does not exist + end + end + end + end + end + + describe ':after_yield' do + it 'removes the lock after yielding to the worker' do + jid = UntilExecutedJob.perform_async + item = Sidekiq::Queue.new(queue).find_job(jid).item + + middleware.call('UntilExecutedJob', item, queue) do + Sidekiq.redis do |conn| + conn.keys('unique:*').each do |key| + expect(conn.get(key)).to eq(-2) # key does not exist + end + end + end + end + end + end +end diff --git a/spec/lib/sidekiq_unique_jobs/sidekiq_unique_ext_spec.rb b/spec/unit/sidekiq_unique_ext_spec.rb similarity index 54% rename from spec/lib/sidekiq_unique_jobs/sidekiq_unique_ext_spec.rb rename to spec/unit/sidekiq_unique_ext_spec.rb index cb4a33018..27da3a1db 100644 --- a/spec/lib/sidekiq_unique_jobs/sidekiq_unique_ext_spec.rb +++ b/spec/unit/sidekiq_unique_ext_spec.rb @@ -7,71 +7,65 @@ require 'sidekiq_unique_jobs/client/middleware' require 'sidekiq_unique_jobs/sidekiq_unique_ext' -RSpec.describe 'Sidekiq::Api' do +RSpec.describe 'Sidekiq::Api', redis: :redis do let(:item) do - { 'class' => JustAWorker, + { 'class' => 'JustAWorker', 'queue' => 'testqueue', 'args' => [foo: 'bar'] } end def unique_key SidekiqUniqueJobs::UniqueArgs.digest( - 'class' => JustAWorker, + 'class' => 'JustAWorker', 'queue' => 'testqueue', 'args' => [foo: 'bar'], 'at' => (Date.today + 1).to_time.to_i, ) end - def schedule_job - JustAWorker.perform_in(60 * 60 * 3, foo: 'bar') - end - - def perform_async - JustAWorker.perform_async(foo: 'bar') - end - describe Sidekiq::SortedEntry::UniqueExtension do it 'deletes uniqueness lock on delete' do - expect(schedule_job).to be_truthy + expect(JustAWorker.perform_in(60 * 60 * 3, foo: 'bar')).to be_truthy + Sidekiq.redis do |conn| + expect(conn.keys).to include( + 'uniquejobs:863b7cb639bd71c828459b97788b2ada:EXISTS', + 'uniquejobs:863b7cb639bd71c828459b97788b2ada:GRABBED', + ) + end Sidekiq::ScheduledSet.new.each(&:delete) Sidekiq.redis do |conn| - expect(conn.exists(unique_key)).to be_falsy + expect(conn.keys).to match_array([]) end - expect(schedule_job).to be_truthy + expect(JustAWorker.perform_in(60 * 60 * 3, boo: 'far')).to be_truthy end end describe Sidekiq::Job::UniqueExtension do it 'deletes uniqueness lock on delete' do - jid = perform_async + jid = JustAWorker.perform_async(roo: 'baf') + expect(SidekiqUniqueJobs::Util.keys).not_to match_array([]) Sidekiq::Queue.new('testqueue').find_job(jid).delete - Sidekiq.redis do |conn| - expect(conn.exists(unique_key)).to be_falsy - end - expect(true).to be_truthy + expect(SidekiqUniqueJobs::Util.keys).to match_array([]) end end describe Sidekiq::Queue::UniqueExtension do it 'deletes uniqueness locks on clear' do - perform_async + JustAWorker.perform_async(oob: 'far') + expect(SidekiqUniqueJobs::Util.keys).not_to match_array([]) Sidekiq::Queue.new('testqueue').clear - Sidekiq.redis do |conn| - expect(conn.exists(unique_key)).to be_falsy - end + expect(SidekiqUniqueJobs::Util.keys).to match_array([]) end end describe Sidekiq::JobSet::UniqueExtension do it 'deletes uniqueness locks on clear' do - schedule_job + JustAWorker.perform_in(60 * 60 * 3, roo: 'fab') + expect(SidekiqUniqueJobs::Util.keys).not_to match_array([]) Sidekiq::JobSet.new('schedule').clear - Sidekiq.redis do |conn| - expect(conn.exists(unique_key)).to be_falsy - end + expect(SidekiqUniqueJobs::Util.keys).to match_array([]) end end end diff --git a/spec/lib/sidekiq_unique_jobs/sidekiq_unique_jobs_spec.rb b/spec/unit/sidekiq_unique_jobs_spec.rb similarity index 100% rename from spec/lib/sidekiq_unique_jobs/sidekiq_unique_jobs_spec.rb rename to spec/unit/sidekiq_unique_jobs_spec.rb diff --git a/spec/lib/sidekiq_unique_jobs/timeout_calculator_spec.rb b/spec/unit/timeout/calculator_spec.rb similarity index 82% rename from spec/lib/sidekiq_unique_jobs/timeout_calculator_spec.rb rename to spec/unit/timeout/calculator_spec.rb index ffc47cef0..619ee96f7 100644 --- a/spec/lib/sidekiq_unique_jobs/timeout_calculator_spec.rb +++ b/spec/unit/timeout/calculator_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe SidekiqUniqueJobs::TimeoutCalculator do +RSpec.describe SidekiqUniqueJobs::Timeout::Calculator do let(:calculator) { described_class.new('class' => worker_class, 'at' => schedule_time) } let(:worker_class) { 'MyUniqueJob' } let(:schedule_time) { nil } @@ -17,13 +17,6 @@ it { is_expected.to respond_to(:seconds) } end - describe '.for_item' do - it 'initializes a new calculator' do - expect(described_class).to receive(:new).with('WAT') - described_class.for_item('WAT') - end - end - describe '#time_until_scheduled' do subject { calculator.time_until_scheduled } @@ -43,14 +36,15 @@ end end - describe '#worker_class_queue_lock_expiration' do - subject { calculator.worker_class_queue_lock_expiration } + describe '#worker_class_run_lock_expiration' do + subject { calculator.worker_class_run_lock_expiration } + let(:worker_class) { 'LongRunningRunLockExpirationJob' } - it { is_expected.to eq(7_200) } + it { is_expected.to eq(3_600) } end - describe '#worker_class_run_lock_expiration' do - subject { calculator.worker_class_run_lock_expiration } + describe '#worker_class_lock_expiration' do + subject { calculator.worker_class_lock_expiration } let(:worker_class) { 'LongRunningJob' } it { is_expected.to eq(7_200) } diff --git a/spec/unit/timeout/queue_lock_spec.rb b/spec/unit/timeout/queue_lock_spec.rb new file mode 100644 index 000000000..772577271 --- /dev/null +++ b/spec/unit/timeout/queue_lock_spec.rb @@ -0,0 +1,97 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe SidekiqUniqueJobs::Timeout::QueueLock do + let(:calculator) { described_class.new(item) } + + let(:lock_expiration) { nil } + let(:lock_timeout) { nil } + let(:item) do + { + 'class' => 'JustAWorker', + 'lock_expiration' => lock_expiration, + 'lock_timeout' => lock_timeout, + } + end + + describe 'public api' do + subject { calculator } + it { is_expected.to respond_to(:lock_expiration) } + it { is_expected.to respond_to(:lock_timeout) } + end + + describe '#lock_expiration' do + subject { calculator.lock_expiration } + + let(:time_until_scheduled) { 10 } + let(:worker_class_lock_expiration) { nil } + let(:worker_class_queue_lock_expiration) { nil } + + before do + allow(calculator).to receive(:time_until_scheduled).and_return(time_until_scheduled) + allow(calculator).to receive(:worker_class_lock_expiration).and_return(worker_class_lock_expiration) + allow(calculator).to receive(:worker_class_queue_lock_expiration).and_return(worker_class_queue_lock_expiration) + end + + context 'when argument hash contains `lock_expiration: 10' do + let(:lock_expiration) { 10 } + + it { is_expected.to eq(20) } + end + + context 'when worker is configured with `lock_expiration: 20`' do + let(:worker_class_lock_expiration) { 20 } + + context 'when item["lock_expiration"] is nil' do + it { is_expected.to eq(nil) } + end + + context 'when item["lock_expiration"] is not present' do + let(:item) do + { + 'class' => 'JustAWorker', + 'lock_timeout' => lock_timeout, + } + end + + it { is_expected.to eq(30) } + end + end + + context 'when worker is configured with `queu_lock_expiration: 30`' do + let(:worker_class_queue_lock_expiration) { 30 } + + context 'when item["lock_expiration"] is nil' do + it { is_expected.to eq(nil) } + end + + context 'when item["lock_expiration"] is not present' do + let(:item) do + { + 'class' => 'JustAWorker', + 'lock_timeout' => lock_timeout, + } + end + it { is_expected.to eq(40) } + end + end + + context 'without further configuration' do + context 'when item["lock_expiration"] is nil' do + it { is_expected.to eq(nil) } + end + + context 'when item["lock_expiration"] is not present' do + let(:item) do + { + 'class' => 'JustAWorker', + 'lock_timeout' => lock_timeout, + } + end + + it { is_expected.to eq(1_810) } + end + end + end +end diff --git a/spec/unit/timeout/run_lock_spec.rb b/spec/unit/timeout/run_lock_spec.rb new file mode 100644 index 000000000..80deffd64 --- /dev/null +++ b/spec/unit/timeout/run_lock_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe SidekiqUniqueJobs::Timeout::RunLock do + let(:calculator) { described_class.new(item) } + let(:lock_expiration) { nil } + let(:lock_timeout) { nil } + let(:item) do + { + 'class' => 'JustAWorker', + 'lock_expiration' => lock_expiration, + 'lock_timeout' => lock_timeout, + } + end + + describe 'public api' do + subject { calculator } + it { is_expected.to respond_to(:lock_expiration) } + it { is_expected.to respond_to(:lock_timeout) } + end + + describe '#lock_expiration' do + subject { calculator.lock_expiration } + + let(:time_until_scheduled) { 10 } + let(:worker_class_lock_expiration) { nil } + let(:worker_class_run_lock_expiration) { nil } + + before do + allow(calculator).to receive(:time_until_scheduled).and_return(time_until_scheduled) + allow(calculator).to receive(:worker_class_lock_expiration).and_return(worker_class_lock_expiration) + allow(calculator).to receive(:worker_class_run_lock_expiration).and_return(worker_class_run_lock_expiration) + end + + context 'when argument hash contains `lock_expiration: 10' do + let(:lock_expiration) { 10 } + + it { is_expected.to eq(10) } + end + + context 'when worker is configured with `lock_expiration: 20`' do + let(:worker_class_lock_expiration) { 20 } + + it { is_expected.to eq(20) } + end + + context 'when worker is configured with `queu_lock_expiration: 30`' do + let(:worker_class_run_lock_expiration) { 30 } + + it { is_expected.to eq(30) } + end + + context 'without further configuration' do + it { is_expected.to eq(60) } + end + end +end diff --git a/spec/lib/sidekiq_unique_jobs/unique_args_spec.rb b/spec/unit/unique_args_spec.rb similarity index 100% rename from spec/lib/sidekiq_unique_jobs/unique_args_spec.rb rename to spec/unit/unique_args_spec.rb diff --git a/spec/unit/unlockable_spec.rb b/spec/unit/unlockable_spec.rb new file mode 100644 index 000000000..a785bb919 --- /dev/null +++ b/spec/unit/unlockable_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe SidekiqUniqueJobs::Unlockable, redis: :redis do + def item_with_digest + SidekiqUniqueJobs::UniqueArgs.digest(item) + item + end + let(:item) do + { 'class' => MyUniqueJob, + 'queue' => 'customqueue', + 'args' => [1, 2] } + end + + let(:unique_digest) { item_with_digest[SidekiqUniqueJobs::UNIQUE_DIGEST_KEY] } + + describe '.unlock' do + subject { described_class.unlock(item_with_digest) } + + let(:expected_keys) do + %W[#{unique_digest}:EXISTS #{unique_digest}:GRABBED] + end + + specify do + expect(SidekiqUniqueJobs::Util.keys.count).to eq(0) + Sidekiq::Client.push(item_with_digest) + + expect(SidekiqUniqueJobs::Util.keys.count).to eq(2) + + subject + + expect(SidekiqUniqueJobs::Util.keys.count).to eq(2) + end + end + + describe '.delete!' do + subject { described_class.delete!(item_with_digest) } + + specify do + expect(SidekiqUniqueJobs::Util.keys.count).to eq(0) + Sidekiq::Client.push(item_with_digest) + + expect(SidekiqUniqueJobs::Util.keys.count).to eq(2) + + subject + + expect(SidekiqUniqueJobs::Util.keys.count).to eq(0) + end + end +end diff --git a/spec/unit/util_spec.rb b/spec/unit/util_spec.rb new file mode 100644 index 000000000..8b571ac47 --- /dev/null +++ b/spec/unit/util_spec.rb @@ -0,0 +1,96 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe SidekiqUniqueJobs::Util, redis: :redis do + let(:item_hash) do + { + 'class' => 'MyUniqueJob', + 'args' => [[1, 2]], + 'at' => 1_492_341_850.358196, + 'retry' => true, + 'queue' => 'customqueue', + 'unique' => :until_executed, + 'expiration' => 7200, + 'retry_count' => 10, + 'jid' => jid, + 'created_at' => 1_492_341_790.358217, + } + end + let!(:item) do + my_item = item_hash.dup + SidekiqUniqueJobs::UniqueArgs.new(my_item).unique_digest + my_item + end + + let(:unique_key) { item['unique_digest'] } + let(:jid) { 'e3049b05b0bd9c809182bbe0' } + let(:lock) { SidekiqUniqueJobs::Lock.new(item) } + let(:expected_keys) do + %W[ + #{unique_key}:EXISTS + #{unique_key}:GRABBED + ] + end + + describe '.keys' do + subject { described_class.keys } + before do + expect(described_class.keys).to match_array([]) + lock.lock(0) + end + + it { is_expected.to match_array(expected_keys) } + end + + describe '.del' do + subject { described_class.del(pattern, 100, false) } + + before do + lock.lock(0) + expect(described_class.keys).to match_array(expected_keys) + end + + context 'when pattern is a wildcard' do + let(:pattern) { described_class::SCAN_PATTERN } + + it { is_expected.to eq(2) } + end + + context 'when pattern is a specific key' do + let(:pattern) { unique_key } + + it { is_expected.to eq(2) } + end + + after do + expect(described_class.keys).to match_array([]) + lock.unlock + lock.delete! + end + end + + describe '.prefix' do + subject { described_class.send(:prefix, key) } + + let(:key) { 'key' } + + context 'when prefix is configured' do + before { allow(SidekiqUniqueJobs.config).to receive(:unique_prefix).and_return('test-uniqueness') } + + it { is_expected.to eq('test-uniqueness:key') } + + context 'when key is already prefixed' do + let(:key) { 'test-uniqueness:key' } + + it { is_expected.to eq('test-uniqueness:key') } + end + end + + context 'when .unique_prefix is nil?' do + before { allow(SidekiqUniqueJobs.config).to receive(:unique_prefix).and_return(nil) } + + it { is_expected.to eq('key') } + end + end +end