From 692508a49cf8359fd31df6f0735a580e21b01641 Mon Sep 17 00:00:00 2001 From: Mikael Henriksson Date: Sun, 16 Apr 2017 19:16:20 +0200 Subject: [PATCH 1/7] Add the possibility to clear the hash We store the jids in the hash. While this is now working and continuously reducing the number of keys for every unlock we were missing a way to clear the existing mess. --- .editorconfig | 4 +- .rubocop.yml | 2 +- CHANGELOG.md | 5 + lib/sidekiq-unique-jobs.rb | 6 +- lib/sidekiq/simulator.rb | 7 +- lib/sidekiq_unique_jobs/cli.rb | 32 +++- .../lock/until_executed.rb | 20 +- lib/sidekiq_unique_jobs/scripts.rb | 14 +- .../scripts/acquire_lock.rb | 45 +++++ .../scripts/release_lock.rb | 46 +++++ lib/sidekiq_unique_jobs/testing.rb | 2 +- lib/sidekiq_unique_jobs/unique_args.rb | 6 +- lib/sidekiq_unique_jobs/unlockable.rb | 22 +-- lib/sidekiq_unique_jobs/util.rb | 23 ++- rails_example/.env | 2 +- rails_example/Gemfile | 1 + rails_example/Procfile | 2 +- rails_example/app/workers/simple_worker.rb | 2 +- .../workers/slow_until_executing_worker.rb | 2 +- .../app/workers/spawn_simple_worker.rb | 2 +- .../app/workers/while_executing_worker.rb | 12 ++ .../app/workers/without_argument_worker.rb | 2 +- sidekiq-unique-jobs.gemspec | 1 + spec/jobs/another_unique_job.rb | 2 +- spec/jobs/my_job.rb | 2 +- spec/jobs/unique_job_with_filter_method.rb | 2 +- spec/jobs/unique_on_all_queues_job.rb | 2 +- spec/jobs/until_executed_job.rb | 2 +- spec/jobs/while_executing_job.rb | 2 +- spec/lib/sidekiq_unique_jobs/cli_spec.rb | 178 ++++++++++++++++++ .../client/middleware_spec.rb | 10 +- .../lock/until_and_while_executing_spec.rb | 2 - .../sidekiq_unique_jobs/script_mock_spec.rb | 4 +- .../scripts/acquire_lock_spec.rb | 50 +++++ spec/lib/sidekiq_unique_jobs/scripts_spec.rb | 12 +- .../server/middleware_spec.rb | 5 - .../sidekiq_testing_enabled_spec.rb | 19 -- .../sidekiq_unique_ext_spec.rb | 5 - .../sidekiq_unique_jobs/unlockable_spec.rb | 1 - spec/lib/sidekiq_unique_jobs/util_spec.rb | 80 +++++--- spec/spec_helper.rb | 42 ++++- spec/support/matchers/redis_matchers.rb | 22 ++- 42 files changed, 547 insertions(+), 155 deletions(-) create mode 100644 lib/sidekiq_unique_jobs/scripts/acquire_lock.rb create mode 100644 lib/sidekiq_unique_jobs/scripts/release_lock.rb create mode 100644 rails_example/app/workers/while_executing_worker.rb create mode 100644 spec/lib/sidekiq_unique_jobs/cli_spec.rb create mode 100644 spec/lib/sidekiq_unique_jobs/scripts/acquire_lock_spec.rb diff --git a/.editorconfig b/.editorconfig index 65d44abc7..aaeaf0ab6 100644 --- a/.editorconfig +++ b/.editorconfig @@ -10,5 +10,5 @@ insert_final_newline = true indent_style = space indent_size = 2 -[*.md] -trim_trailing_whitespace = true +[*.{md,slim,haml}] +trim_trailing_whitespace = false diff --git a/.rubocop.yml b/.rubocop.yml index bca4fb4e8..c154faa64 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -22,7 +22,7 @@ Metrics/CyclomaticComplexity: Max: 7 Metrics/LineLength: - Max: 108 + Max: 120 Metrics/MethodLength: Max: 13 diff --git a/CHANGELOG.md b/CHANGELOG.md index efa6ea934..d3eed6652 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +## v5.0.1 + +- Added a command line util for cleaning out expired keys +- Use `SidekiqUniqueJobs.logger` instead of spreading out `Sidekiq.logger` everywhere. + ## v5.0.0 - Only support Sidekiq >= 4 diff --git a/lib/sidekiq-unique-jobs.rb b/lib/sidekiq-unique-jobs.rb index f3df84619..d4d9e438f 100644 --- a/lib/sidekiq-unique-jobs.rb +++ b/lib/sidekiq-unique-jobs.rb @@ -26,10 +26,14 @@ def config default_queue_lock_expiration: 30 * 60, default_run_lock_expiration: 60, default_lock: :while_executing, - redis_test_mode: :redis # :mock + redis_test_mode: :redis, # :mock ) end + def logger + Sidekiq.logger + end + def default_lock config.default_lock end diff --git a/lib/sidekiq/simulator.rb b/lib/sidekiq/simulator.rb index f16878db7..7daba422a 100644 --- a/lib/sidekiq/simulator.rb +++ b/lib/sidekiq/simulator.rb @@ -3,6 +3,9 @@ module Sidekiq class Simulator + extend Forwardable + def_delegator SidekiqUniqueJobs, :logger + attr_reader :queues, :launcher def self.process_queue(queue) @@ -67,9 +70,5 @@ def sidekiq_options(queues = []) verbose: false, logfile: './tmp/sidekiq.log' } end - - def logger - @logger ||= Sidekiq.logger - end end end diff --git a/lib/sidekiq_unique_jobs/cli.rb b/lib/sidekiq_unique_jobs/cli.rb index d6a2b8681..d4280fbd9 100644 --- a/lib/sidekiq_unique_jobs/cli.rb +++ b/lib/sidekiq_unique_jobs/cli.rb @@ -2,30 +2,52 @@ module SidekiqUniqueJobs class Cli < Thor + # def initialize(argv, stdin = STDIN, stdout = STDOUT, stderr = STDERR, kernel = Kernel) + # @argv, @stdin, @stdout, @stderr, @kernel = argv, stdin, stdout, stderr, kernel + # end + + def self.banner(command, _namespace = nil, _subcommand = false) + "jobs #{@package_name} #{command.usage}" + end + desc 'keys PATTERN', 'list all unique keys and their expiry time' option :count, aliases: :c, type: :numeric, default: 1000, desc: 'The max number of keys to return' def keys(pattern) - Util.keys(pattern, options[:count]) + keys = Util.keys(pattern, options[:count]) + say "Found #{keys.size} keys matching '#{pattern}':" + print_in_columns(keys) if keys.any? end desc 'del PATTERN', 'deletes unique keys from redis by pattern' option :dry_run, aliases: :d, type: :boolean, desc: 'set to false to perform deletion' option :count, aliases: :c, type: :numeric, default: 1000, desc: 'The max number of keys to return' def del(pattern) - Util.del(pattern, options[:count], options[:dry_run]) + deleted_count = Util.del(pattern, options[:count], options[:dry_run]) + 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 - puts "Use `keys '*', 1000 to display the first 1000 unique keys matching '*'" - puts "Use `del '*', 1000, true (default) to see how many keys would be deleted for the pattern '*'" - puts "Use `del '*', 1000, false to delete the first 1000 keys matching '*'" + say "Use `keys '*', 1000 to display the first 1000 unique keys matching '*'" + say "Use `del '*', 1000, true (default) to see how many keys would be deleted for the pattern '*'" + say "Use `del '*', 1000, false to delete the first 1000 keys matching '*'" Object.include SidekiqUniqueJobs::Util console_class.start end private + def logger + SidekiqUniqueJobs.logger + end + def console_class require 'pry' Pry diff --git a/lib/sidekiq_unique_jobs/lock/until_executed.rb b/lib/sidekiq_unique_jobs/lock/until_executed.rb index b59f3b53f..dcdae3251 100644 --- a/lib/sidekiq_unique_jobs/lock/until_executed.rb +++ b/lib/sidekiq_unique_jobs/lock/until_executed.rb @@ -35,25 +35,17 @@ def unlock(scope) unlock_by_key(unique_key, item[JID_KEY], redis_pool) end - # rubocop:disable MethodLength def lock(scope) if scope.to_sym != :client raise ArgumentError, "#{scope} middleware can't #{__method__} #{unique_key}" end - result = Scripts.call(:acquire_lock, redis_pool, - keys: [unique_key], - argv: [item[JID_KEY], max_lock_time]) - case result - when 1 - logger.debug { "successfully locked #{unique_key} for #{max_lock_time} seconds" } - true - when 0 - logger.debug { "failed to acquire lock for #{unique_key}" } - false - else - raise "#{__method__} returned an unexpected value (#{result})" - end + Scripts::AcquireLock.execute( + redis_pool, + unique_key, + item[JID_KEY], + max_lock_time + ) end # rubocop:enable MethodLength diff --git a/lib/sidekiq_unique_jobs/scripts.rb b/lib/sidekiq_unique_jobs/scripts.rb index b8a72aa7f..9c470c30d 100644 --- a/lib/sidekiq_unique_jobs/scripts.rb +++ b/lib/sidekiq_unique_jobs/scripts.rb @@ -1,9 +1,15 @@ 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) + 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 @@ -14,11 +20,7 @@ module Scripts module_function extend SingleForwardable - def_delegator :SidekiqUniqueJobs, :connection - - def logger - Sidekiq.logger - end + def_delegators :SidekiqUniqueJobs, :connection, :logger def call(file_name, redis_pool, options = {}) # rubocop:disable MethodLength connection(redis_pool) do |redis| diff --git a/lib/sidekiq_unique_jobs/scripts/acquire_lock.rb b/lib/sidekiq_unique_jobs/scripts/acquire_lock.rb new file mode 100644 index 000000000..bdcfeba68 --- /dev/null +++ b/lib/sidekiq_unique_jobs/scripts/acquire_lock.rb @@ -0,0 +1,45 @@ +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 new file mode 100644 index 000000000..3b5db1d78 --- /dev/null +++ b/lib/sidekiq_unique_jobs/scripts/release_lock.rb @@ -0,0 +1,46 @@ +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? + + @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 "#{calling_method} returned an unexpected value (#{result})" + end + end + end + end +end diff --git a/lib/sidekiq_unique_jobs/testing.rb b/lib/sidekiq_unique_jobs/testing.rb index 4c72214dc..6cf986f60 100644 --- a/lib/sidekiq_unique_jobs/testing.rb +++ b/lib/sidekiq_unique_jobs/testing.rb @@ -25,7 +25,7 @@ class << self module Testing def call_ext(file_name, redis_pool, options = {}) - if SidekiqUniqueJobs.config.redis_test_mode == :mock + if SidekiqUniqueJobs.mocked? SidekiqUniqueJobs::ScriptMock.call(file_name, redis_pool, options) else call_orig(file_name, redis_pool, options) diff --git a/lib/sidekiq_unique_jobs/unique_args.rb b/lib/sidekiq_unique_jobs/unique_args.rb index fa9c7795f..3cf532780 100644 --- a/lib/sidekiq_unique_jobs/unique_args.rb +++ b/lib/sidekiq_unique_jobs/unique_args.rb @@ -19,10 +19,10 @@ def self.digest(item) def initialize(job) Sidekiq::Logging.with_context(CLASS_NAME) do @item = job - @worker_class ||= worker_class_constantize(@item[CLASS_KEY]) + @worker_class ||= worker_class_constantize(@item[CLASS_KEY]) @item[UNIQUE_PREFIX_KEY] ||= unique_prefix - @item[UNIQUE_ARGS_KEY] = unique_args(@item[ARGS_KEY]) # SIC! Calculate unique_args unconditionally - @item[UNIQUE_DIGEST_KEY] = unique_digest + @item[UNIQUE_ARGS_KEY] = unique_args(@item[ARGS_KEY]) + @item[UNIQUE_DIGEST_KEY] = unique_digest end end diff --git a/lib/sidekiq_unique_jobs/unlockable.rb b/lib/sidekiq_unique_jobs/unlockable.rb index cf3a6839b..a4ae0883f 100644 --- a/lib/sidekiq_unique_jobs/unlockable.rb +++ b/lib/sidekiq_unique_jobs/unlockable.rb @@ -7,26 +7,12 @@ def unlock(item) end def unlock_by_key(unique_key, jid, redis_pool = nil) - result = Scripts.call(:release_lock, redis_pool, keys: [unique_key], argv: [jid]) - after_unlock(result, __method__, unique_key, jid) + return false unless Scripts::ReleaseLock.execute(redis_pool, unique_key, jid) + after_unlock(jid) end - def after_unlock(result, calling_method, unique_key, jid) # rubocop:disable Metrics/MethodLength + def after_unlock(jid) ensure_job_id_removed(jid) - - 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 "#{calling_method} returned an unexpected value (#{result})" - end end def ensure_job_id_removed(jid) @@ -34,7 +20,7 @@ def ensure_job_id_removed(jid) end def logger - Sidekiq.logger + SidekiqUniqueJobs.logger end end end diff --git a/lib/sidekiq_unique_jobs/util.rb b/lib/sidekiq_unique_jobs/util.rb index 83dcaac83..fe87b1599 100644 --- a/lib/sidekiq_unique_jobs/util.rb +++ b/lib/sidekiq_unique_jobs/util.rb @@ -18,7 +18,7 @@ def unique_key(jid) end def del(pattern = SCAN_PATTERN, count = 0, dry_run = true) - raise 'Please provide a number of keys to delete greater than zero' if count.zero? + raise ArgumentError, 'Please provide a number of keys to delete greater than zero' if count.zero? logger.debug { "Deleting keys by: #{pattern}" } keys, time = timed { keys(pattern, count) } logger.debug { "#{keys.size} matching keys found in #{time} sec." } @@ -32,6 +32,24 @@ 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 + removed_keys = {} + connection do |conn| + conn.hgetall(SidekiqUniqueJobs::HASH_KEY).each do |jid, unique_key| + next if conn.get(unique_key) + conn.hdel(SidekiqUniqueJobs::HASH_KEY, jid) + removed_keys[jid] = unique_key + end + end + removed_keys + end + def keys_by_scan(pattern, count) connection { |conn| conn.scan_each(match: prefix(pattern), count: count).to_a } end @@ -72,6 +90,7 @@ def prefix_keys(keys) def prefix(key) return key if unique_prefix.nil? + return key if key.start_with?("#{unique_prefix}:") "#{unique_prefix}:#{key}" end @@ -92,7 +111,7 @@ def redis_keys_method end def logger - Sidekiq.logger + SidekiqUniqueJobs.logger end end end diff --git a/rails_example/.env b/rails_example/.env index 918d09847..3ae005a73 100644 --- a/rails_example/.env +++ b/rails_example/.env @@ -3,7 +3,7 @@ APP_DOMAIN=localhost APP_WEB_URL=http://localhost:3000 APP_CABLE_URL=ws://localhost:28080 WEB_CONSOLE_WHITELISTED_IPS=127.0.0.1 ::1 127.0.0.0/8 ::1 -DB_HOST=postgres +DB_HOST=localhost DB_PORT=5432 DB_USERNAME=mhenrixon DB_PASSWORD= diff --git a/rails_example/Gemfile b/rails_example/Gemfile index a7153857a..f7673f4dd 100644 --- a/rails_example/Gemfile +++ b/rails_example/Gemfile @@ -25,6 +25,7 @@ gem 'sidekiq-unique-jobs', path: '../' gem 'sinatra', github: 'sinatra/sinatra' group :development do + gem 'foreman' gem 'web-console' end diff --git a/rails_example/Procfile b/rails_example/Procfile index b28132370..e36154a63 100644 --- a/rails_example/Procfile +++ b/rails_example/Procfile @@ -1,2 +1,2 @@ web: bundle exec rails server -p $PORT -worker: bundle exec sidekiq +worker: bundle exec sidekiq -C config/sidekiq.yml diff --git a/rails_example/app/workers/simple_worker.rb b/rails_example/app/workers/simple_worker.rb index 4416eecf2..ffb03947e 100644 --- a/rails_example/app/workers/simple_worker.rb +++ b/rails_example/app/workers/simple_worker.rb @@ -8,7 +8,7 @@ class SimpleWorker def perform(some_args) Sidekiq::Logging.with_context(self.class.name) do - Sidekiq.logger.debug { "#{__method__}(#{some_args})" } + SidekiqUniqueJobs.logger.debug { "#{__method__}(#{some_args})" } end sleep 1 end diff --git a/rails_example/app/workers/slow_until_executing_worker.rb b/rails_example/app/workers/slow_until_executing_worker.rb index 390cb1860..dae20bc34 100644 --- a/rails_example/app/workers/slow_until_executing_worker.rb +++ b/rails_example/app/workers/slow_until_executing_worker.rb @@ -8,7 +8,7 @@ class SlowUntilExecutingWorker def perform(some_args) Sidekiq::Logging.with_context(self.class.name) do - Sidekiq.logger.debug { "#{__method__}(#{some_args})" } + SidekiqUniqueJobs.logger.debug { "#{__method__}(#{some_args})" } end sleep 15 end diff --git a/rails_example/app/workers/spawn_simple_worker.rb b/rails_example/app/workers/spawn_simple_worker.rb index 44368c9cb..705286635 100644 --- a/rails_example/app/workers/spawn_simple_worker.rb +++ b/rails_example/app/workers/spawn_simple_worker.rb @@ -3,7 +3,7 @@ class SpawnSimpleWorker def perform(spawn_arg) Sidekiq::Logging.with_context(self.class.name) do - Sidekiq.logger.debug { "#{__method__}(#{spawn_arg})" } + logger.debug { "#{__method__}(#{spawn_arg})" } end SimpleWorker.perform_async spawn_arg end diff --git a/rails_example/app/workers/while_executing_worker.rb b/rails_example/app/workers/while_executing_worker.rb new file mode 100644 index 000000000..965455d8d --- /dev/null +++ b/rails_example/app/workers/while_executing_worker.rb @@ -0,0 +1,12 @@ +class WhileExecutingWorker + include Sidekiq::Worker + + sidekiq_options unique: :while_executing + + def perform(_one, _two) + Sidekiq::Logging.with_context(self.class.name) do + logger.debug { "#{__method__}(#{some_args})" } + end + sleep 10 + end +end diff --git a/rails_example/app/workers/without_argument_worker.rb b/rails_example/app/workers/without_argument_worker.rb index afbb7639b..50386abf0 100644 --- a/rails_example/app/workers/without_argument_worker.rb +++ b/rails_example/app/workers/without_argument_worker.rb @@ -5,7 +5,7 @@ class WithoutArgumentWorker def perform Sidekiq::Logging.with_context(self.class.name) do - Sidekiq.logger.debug { __method__.to_s } + logger.debug { __method__.to_s } end sleep 20 end diff --git a/sidekiq-unique-jobs.gemspec b/sidekiq-unique-jobs.gemspec index a54351061..13a68712f 100644 --- a/sidekiq-unique-jobs.gemspec +++ b/sidekiq-unique-jobs.gemspec @@ -23,5 +23,6 @@ Gem::Specification.new do |gem| gem.add_development_dependency 'timecop' gem.add_development_dependency 'yard' gem.add_development_dependency 'gem-release' + gem.add_development_dependency 'aruba' gem.add_development_dependency 'codeclimate-test-reporter', '~> 1.0.0' end diff --git a/spec/jobs/another_unique_job.rb b/spec/jobs/another_unique_job.rb index 2450c8347..9a4d926d4 100644 --- a/spec/jobs/another_unique_job.rb +++ b/spec/jobs/another_unique_job.rb @@ -4,7 +4,7 @@ class AnotherUniqueJob unique: :until_executed sidekiq_retries_exhausted do |msg| - Sidekiq.logger.warn "Failed #{msg['class']} with #{msg['args']}: #{msg['error_message']}" + logger.warn "Failed #{msg['class']} with #{msg['args']}: #{msg['error_message']}" end def perform(*) diff --git a/spec/jobs/my_job.rb b/spec/jobs/my_job.rb index 4f1a54405..ef75138ca 100644 --- a/spec/jobs/my_job.rb +++ b/spec/jobs/my_job.rb @@ -3,7 +3,7 @@ class MyJob sidekiq_options queue: :working, retry: 1, backtrace: 10, unique: false sidekiq_retries_exhausted do |msg| - Sidekiq.logger.warn "Failed #{msg['class']} with #{msg['args']}: #{msg['error_message']}" + logger.warn "Failed #{msg['class']} with #{msg['args']}: #{msg['error_message']}" end def perform(*) diff --git a/spec/jobs/unique_job_with_filter_method.rb b/spec/jobs/unique_job_with_filter_method.rb index ac410a45a..ce4729e8f 100644 --- a/spec/jobs/unique_job_with_filter_method.rb +++ b/spec/jobs/unique_job_with_filter_method.rb @@ -4,7 +4,7 @@ class UniqueJobWithFilterMethod unique: :while_executing, unique_args: :filtered_args sidekiq_retries_exhausted do |msg| - Sidekiq.logger.warn "Failed #{msg['class']} with #{msg['args']}: #{msg['error_message']}" + logger.warn "Failed #{msg['class']} with #{msg['args']}: #{msg['error_message']}" end def perform(*) diff --git a/spec/jobs/unique_on_all_queues_job.rb b/spec/jobs/unique_on_all_queues_job.rb index 7f7a5cd61..bfc948041 100644 --- a/spec/jobs/unique_on_all_queues_job.rb +++ b/spec/jobs/unique_on_all_queues_job.rb @@ -4,7 +4,7 @@ class UniqueOnAllQueuesJob unique: :until_executed, unique_on_all_queues: true sidekiq_retries_exhausted do |msg| - Sidekiq.logger.warn "Failed #{msg['class']} with #{msg['args']}: #{msg['error_message']}" + logger.warn "Failed #{msg['class']} with #{msg['args']}: #{msg['error_message']}" end def perform(*) diff --git a/spec/jobs/until_executed_job.rb b/spec/jobs/until_executed_job.rb index ed4467fca..27e4cd8b2 100644 --- a/spec/jobs/until_executed_job.rb +++ b/spec/jobs/until_executed_job.rb @@ -4,7 +4,7 @@ class UntilExecutedJob sidekiq_options unique: :until_executed sidekiq_retries_exhausted do |msg| - Sidekiq.logger.warn "Failed #{msg['class']} with #{msg['args']}: #{msg['error_message']}" + logger.warn "Failed #{msg['class']} with #{msg['args']}: #{msg['error_message']}" end def perform(*) diff --git a/spec/jobs/while_executing_job.rb b/spec/jobs/while_executing_job.rb index d1886845b..da2fe0682 100644 --- a/spec/jobs/while_executing_job.rb +++ b/spec/jobs/while_executing_job.rb @@ -3,7 +3,7 @@ class WhileExecutingJob sidekiq_options queue: :working, retry: 1, backtrace: 10, unique: :while_executing sidekiq_retries_exhausted do |msg| - Sidekiq.logger.warn "Failed #{msg['class']} with #{msg['args']}: #{msg['error_message']}" + logger.warn "Failed #{msg['class']} with #{msg['args']}: #{msg['error_message']}" end def perform(_) diff --git a/spec/lib/sidekiq_unique_jobs/cli_spec.rb b/spec/lib/sidekiq_unique_jobs/cli_spec.rb new file mode 100644 index 000000000..06041a0e1 --- /dev/null +++ b/spec/lib/sidekiq_unique_jobs/cli_spec.rb @@ -0,0 +1,178 @@ +require 'spec_helper' +require 'thor/runner' + +RSpec.describe SidekiqUniqueJobs::Cli do + describe '#help' do + let(:output) { capture(:stdout) { described_class.start(%w[help]) } } + let(:banner) do + <<~EOS.freeze + 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 + end + + it 'displays help' do + expect(output).to include(banner) + end + + describe '#help del' do + let(:output) { capture(:stdout) { described_class.start(%w[help del]) } } + let(:banner) do + <<~EOS.freeze + Usage: + jobs del PATTERN + + Options: + d, [--dry-run], [--no-dry-run] # set to false to perform deletion + c, [--count=N] # The max number of keys to return + # Default: 1000 + + deletes unique keys from redis by pattern + EOS + end + + it 'displays help about the `del` command' do + expect(output).to eq(banner) + end + end + + describe '#help expire' do + let(:output) { capture(:stdout) { described_class.start(%w[help expire]) } } + + let(:banner) do + <<~EOS.freeze + 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 + <<~EOS.freeze + Usage: + jobs keys PATTERN + + Options: + c, [--count=N] # The max number of keys to return + # Default: 1000 + + list all unique keys and their expiry time + EOS + end + + it 'displays help about the `key` command' do + expect(output).to eq(banner) + end + end + end + + let(:pattern) { '*' } + let(:max_lock_time) { 1 } + let(:unique_key) { 'uniquejobs:abcdefab' } + let(:jid) { 'abcdefab' } + + describe '.keys' do + let(:output) { capture(:stdout) { described_class.start(%w[keys * --count 1000]) } } + + context 'when no keys exist' do + let(:expected) { "Found 0 keys matching '#{pattern}':\n" } + specify { expect(output).to eq(expected) } + end + + context 'when a key exists' do + let(:keys) { ['defghayl', jid, 'poilkij'] } + 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 + end + + after { SidekiqUniqueJobs::Util.del('*', 1000, false) } + + let(:expected) do + "Found 3 keys matching '#{pattern}':\nuniquejobs:poilkij uniquejobs:abcdefab uniquejobs:defghayl\n" + end + specify { expect(output).to eq(expected) } + end + end + + describe '.del' do + let(:expected) do + <<~EOS + Deleted 1 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) + 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) + end + end + + describe '.console' do + let(:expected) do + <<~EOS + Use `keys '*', 1000 to display the first 1000 unique keys matching '*' + Use `del '*', 1000, true (default) to see how many keys would be deleted for the pattern '*' + Use `del '*', 1000, false to delete the first 1000 keys matching '*' + EOS + end + let(:output) { capture(:stdout) { described_class.start(%w[console]) } } + + specify do + expect(Object).to receive(:include).with(SidekiqUniqueJobs::Util).and_return(true) + console = double(:console) + allow_any_instance_of(SidekiqUniqueJobs::Cli).to receive(:console_class).and_return(console) + allow(console).to receive(:start) + expect(output).to eq(expected) + end + end +end diff --git a/spec/lib/sidekiq_unique_jobs/client/middleware_spec.rb b/spec/lib/sidekiq_unique_jobs/client/middleware_spec.rb index 07ece3d83..25dcec02e 100644 --- a/spec/lib/sidekiq_unique_jobs/client/middleware_spec.rb +++ b/spec/lib/sidekiq_unique_jobs/client/middleware_spec.rb @@ -9,14 +9,6 @@ def digest_for(item) end describe 'with real redis' do - before do - if defined?(Sidekiq::Extensions) && Sidekiq::Extensions.respond_to?(:enable_delay!) - Sidekiq::Extensions.enable_delay! - end - Sidekiq.redis = REDIS - Sidekiq.redis(&:flushdb) - end - describe 'when a job is already scheduled' do it 'processes jobs properly' do Sidekiq::Testing.disable! do @@ -288,7 +280,7 @@ def do_it(_arg) end it 'does not log duplicate payload when config turned off' do - expect(Sidekiq.logger).to_not receive(:warn).with(/^payload is not unique/) + expect(SidekiqUniqueJobs.logger).to_not receive(:warn).with(/^payload is not unique/) UntilExecutedJob.sidekiq_options log_duplicate_payload: false diff --git a/spec/lib/sidekiq_unique_jobs/lock/until_and_while_executing_spec.rb b/spec/lib/sidekiq_unique_jobs/lock/until_and_while_executing_spec.rb index d942cfcf9..f2a3d9b4b 100644 --- a/spec/lib/sidekiq_unique_jobs/lock/until_and_while_executing_spec.rb +++ b/spec/lib/sidekiq_unique_jobs/lock/until_and_while_executing_spec.rb @@ -15,8 +15,6 @@ describe '#execute' do before do - Sidekiq.redis(&:flushdb) - Sidekiq::Worker.clear_all subject.lock(:client) end let(:runtime_lock) { SidekiqUniqueJobs::Lock::WhileExecuting.new(item, nil) } diff --git a/spec/lib/sidekiq_unique_jobs/script_mock_spec.rb b/spec/lib/sidekiq_unique_jobs/script_mock_spec.rb index 41d2ad061..88765939c 100644 --- a/spec/lib/sidekiq_unique_jobs/script_mock_spec.rb +++ b/spec/lib/sidekiq_unique_jobs/script_mock_spec.rb @@ -14,7 +14,6 @@ ANOTHER_JID ||= 'anotherjid'.freeze before do - Sidekiq.redis(&:flushdb) SidekiqUniqueJobs.configure do |config| config.redis_test_mode = :mock end @@ -36,7 +35,6 @@ SidekiqUniqueJobs.configure do |config| config.redis_test_mode = :redis end - Sidekiq.redis(&:flushdb) end subject { SidekiqUniqueJobs::Scripts } @@ -54,7 +52,7 @@ def unlock(key = UNIQUE_KEY, jid = JID) specify { expect(lock_for).to eq(1) } specify do expect(lock_for(1)).to eq(1) - expect(Redis) + expect(SidekiqUniqueJobs) .to have_key(UNIQUE_KEY) .for_seconds(1) .with_value('fuckit') diff --git a/spec/lib/sidekiq_unique_jobs/scripts/acquire_lock_spec.rb b/spec/lib/sidekiq_unique_jobs/scripts/acquire_lock_spec.rb new file mode 100644 index 000000000..1e17e1f01 --- /dev/null +++ b/spec/lib/sidekiq_unique_jobs/scripts/acquire_lock_spec.rb @@ -0,0 +1,50 @@ +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_spec.rb b/spec/lib/sidekiq_unique_jobs/scripts_spec.rb index 8760cca6d..9dd3e4bff 100644 --- a/spec/lib/sidekiq_unique_jobs/scripts_spec.rb +++ b/spec/lib/sidekiq_unique_jobs/scripts_spec.rb @@ -6,10 +6,6 @@ ANOTHER_JID ||= 'anotherjid'.freeze context 'class methods' do - before do - Sidekiq.redis(&:flushdb) - Sidekiq::Worker.clear_all - end subject { SidekiqUniqueJobs::Scripts } it { is_expected.to respond_to(:call).with(3).arguments } @@ -34,12 +30,12 @@ def unlock(key = UNIQUE_KEY, jid = JID) context 'when job is unique' do specify { expect(lock_for).to eq(1) } specify do - expect(lock_for(0.5)).to eq(1) - expect(Redis) + expect(lock_for(2)).to eq(1) + expect(SidekiqUniqueJobs) .to have_key(UNIQUE_KEY) .for_seconds(1) - .with_value('fuckit') - sleep 0.5 + .with_value(JID) + sleep 2 expect(lock_for).to eq(1) end diff --git a/spec/lib/sidekiq_unique_jobs/server/middleware_spec.rb b/spec/lib/sidekiq_unique_jobs/server/middleware_spec.rb index 774e6d272..1fbc12a5c 100644 --- a/spec/lib/sidekiq_unique_jobs/server/middleware_spec.rb +++ b/spec/lib/sidekiq_unique_jobs/server/middleware_spec.rb @@ -11,11 +11,6 @@ def digest_for(item) SidekiqUniqueJobs::UniqueArgs.digest(item) end - before do - Sidekiq.redis = REDIS - Sidekiq.redis(&:flushdb) - end - describe '#call' do context 'when unique is disabled' do it 'does not use locking' do diff --git a/spec/lib/sidekiq_unique_jobs/sidekiq_testing_enabled_spec.rb b/spec/lib/sidekiq_unique_jobs/sidekiq_testing_enabled_spec.rb index 96a7b0bef..a1ba9d491 100644 --- a/spec/lib/sidekiq_unique_jobs/sidekiq_testing_enabled_spec.rb +++ b/spec/lib/sidekiq_unique_jobs/sidekiq_testing_enabled_spec.rb @@ -5,25 +5,6 @@ RSpec.describe 'When Sidekiq::Testing is enabled' do describe 'when set to :fake!', sidekiq: :fake do - before do - SidekiqUniqueJobs.configure do |config| - config.redis_test_mode = :redis - end - Sidekiq.redis = REDIS - Sidekiq.redis(&:flushdb) - Sidekiq::Worker.clear_all - if Sidekiq::Testing.respond_to?(:server_middleware) - Sidekiq::Testing.server_middleware do |chain| - chain.add SidekiqUniqueJobs::Server::Middleware - end - end - end - - after do - Sidekiq.redis(&:flushdb) - Sidekiq::Testing.server_middleware(&:clear) if Sidekiq::Testing.respond_to?(:server_middleware) - end - context 'with unique worker' do it 'does not push duplicate messages' do param = 'work' diff --git a/spec/lib/sidekiq_unique_jobs/sidekiq_unique_ext_spec.rb b/spec/lib/sidekiq_unique_jobs/sidekiq_unique_ext_spec.rb index 5da16fc5a..f81fed304 100644 --- a/spec/lib/sidekiq_unique_jobs/sidekiq_unique_ext_spec.rb +++ b/spec/lib/sidekiq_unique_jobs/sidekiq_unique_ext_spec.rb @@ -6,11 +6,6 @@ require 'sidekiq_unique_jobs/sidekiq_unique_ext' RSpec.describe 'Sidekiq::Api' do - before do - Sidekiq.redis = REDIS - Sidekiq.redis(&:flushdb) - end - let(:item) do { 'class' => JustAWorker, 'queue' => 'testqueue', diff --git a/spec/lib/sidekiq_unique_jobs/unlockable_spec.rb b/spec/lib/sidekiq_unique_jobs/unlockable_spec.rb index 3093d86f0..858f9f564 100644 --- a/spec/lib/sidekiq_unique_jobs/unlockable_spec.rb +++ b/spec/lib/sidekiq_unique_jobs/unlockable_spec.rb @@ -1,7 +1,6 @@ require 'spec_helper' RSpec.describe SidekiqUniqueJobs::Unlockable do - before { Sidekiq.redis(&:flushdb) } def item_with_digest SidekiqUniqueJobs::UniqueArgs.digest(item) item diff --git a/spec/lib/sidekiq_unique_jobs/util_spec.rb b/spec/lib/sidekiq_unique_jobs/util_spec.rb index a52764fb4..fcd091534 100644 --- a/spec/lib/sidekiq_unique_jobs/util_spec.rb +++ b/spec/lib/sidekiq_unique_jobs/util_spec.rb @@ -1,47 +1,62 @@ require 'spec_helper' RSpec.describe SidekiqUniqueJobs::Util do - let(:keys) { %w[uniquejobs:keyz] } - - def set(key, value) - described_class.connection do |c| - c.set(key, value) - expect(c.keys('*')).to match_array([key]) - end + let(:job) 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(job) } + let(:unique_key) { unique_args.unique_digest } + let(:jid) { 'e3049b05b0bd9c809182bbe0' } - before(:each) do - Sidekiq.redis = REDIS - Sidekiq.redis(&:flushdb) + def acquire_lock + SidekiqUniqueJobs::Scripts::AcquireLock.execute(nil, unique_key, jid, 1) end describe '.keys' do end describe '.del' do - context 'given a key named "keyz" with value "valz"' do - before do - set('uniquejobs:keyz', 'valz') - end + before do + acquire_lock + end - it 'deletes the keys by pattern' do - expect(described_class.del('*', 100, false)).to eq(1) - 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 pattern' do - expect(described_class.del('keyz', 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 '.prefix' do - context 'when .unique_prefix is nil?' do - it 'does not prefix with unique_prefix' do - allow(SidekiqUniqueJobs.config).to receive(:unique_prefix).and_return(nil) - expect(described_class.prefix('key')).to eq('key') - 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 before do allow(SidekiqUniqueJobs.config).to receive(:unique_prefix).and_return('test-uniqueness') end @@ -49,5 +64,18 @@ def set(key, value) it 'returns a prefixed key' do expect(described_class.prefix('key')).to eq('test-uniqueness:key') end + + context 'when .unique_prefix is nil?' do + it 'does not prefix with unique_prefix' do + allow(SidekiqUniqueJobs.config).to receive(:unique_prefix).and_return(nil) + expect(described_class.prefix('key')).to eq('key') + end + end + + context 'when key is already prefixed' do + it 'does not add another prefix' do + expect(described_class.prefix('test-uniqueness:key')).to eq('test-uniqueness:key') + end + end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index a5369de31..29dc7f80d 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -20,7 +20,7 @@ require 'sidekiq/simulator' Sidekiq::Testing.disable! -Sidekiq.logger.level = "Logger::#{ENV.fetch('LOGLEVEL') { 'error' }.upcase}".constantize +SidekiqUniqueJobs.logger.level = "Logger::#{ENV.fetch('LOGLEVEL') { 'error' }.upcase}".constantize require 'sidekiq/redis_connection' @@ -35,7 +35,8 @@ end Dir[File.join(File.dirname(__FILE__), 'support', '**', '*.rb')].each { |f| require f } -RSpec.configure do |config| + +RSpec.configure do |config| # rubocop:disable BlockLength config.expect_with :rspec do |expectations| expectations.include_chain_clauses_in_custom_matcher_descriptions = true end @@ -49,6 +50,43 @@ 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 } + +def capture(stream) + begin + stream = stream.to_s + eval "$#{stream} = StringIO.new" # rubocop:disable Security/Eval + yield + result = eval("$#{stream}").string # rubocop:disable Security/Eval + ensure + eval("$#{stream} = #{stream.upcase}") # rubocop:disable Security/Eval + end + + result +end diff --git a/spec/support/matchers/redis_matchers.rb b/spec/support/matchers/redis_matchers.rb index e2ec064f2..875ea0b51 100644 --- a/spec/support/matchers/redis_matchers.rb +++ b/spec/support/matchers/redis_matchers.rb @@ -2,18 +2,28 @@ RSpec::Matchers.define :have_key do |unique_key| Sidekiq.redis do |redis| - match do |_actual| - with_value && for_seconds + match do |_unique_jobs| + @value = redis.get(unique_key) + @ttl = redis.ttl(unique_key) + + @value && with_value && for_seconds end chain :with_value do |value = nil| - value.nil? || - redis.get(unique_key) == value + @expected_value = value + @expected_value && @value == @expected_value end chain :for_seconds do |ttl = nil| - ttl.nil? || - redis.ttl(unique_key) == ttl + @expected_ttl = ttl + @expected_ttl && @ttl == @expected_ttl + 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 end end end From a71ea58c4cd8972681d5b8e8ceb4fef85802314c Mon Sep 17 00:00:00 2001 From: Mikael Henriksson Date: Sun, 16 Apr 2017 19:23:59 +0200 Subject: [PATCH 2/7] Fix .codeclimate.yml --- .codeclimate.yml | 31 +------------------------------ 1 file changed, 1 insertion(+), 30 deletions(-) diff --git a/.codeclimate.yml b/.codeclimate.yml index 3e810964b..1978b5039 100644 --- a/.codeclimate.yml +++ b/.codeclimate.yml @@ -1,33 +1,4 @@ --- -engines: - csslint: - enabled: true - duplication: - enabled: true - config: - languages: - - ruby - - javascript - - python - - php - eslint: - enabled: true - fixme: - enabled: true - rubocop: - enabled: true -ratings: - paths: - - "**.css" - - "**.inc" - - "**.js" - - "**.jsx" - - "**.module" - - "**.php" - - "**.py" - - "**.rb" -exclude_paths: - - spec/ engines: bundler-audit: enabled: true @@ -47,7 +18,7 @@ ratings: - "**.rb" exclude_paths: - Gemfile - - *.gemspec + - "*.gemspec" - Appraisals - spec/**/*.rb - gemfiles/**/* From 99a937f60dd7cb531288e6a1cb227b1640bfd54e Mon Sep 17 00:00:00 2001 From: Mikael Henriksson Date: Sun, 16 Apr 2017 19:29:39 +0200 Subject: [PATCH 3/7] Add specs for Releaselock --- .../scripts/release_lock.rb | 5 ++- .../scripts/release_lock_spec.rb | 41 +++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) create mode 100644 spec/lib/sidekiq_unique_jobs/scripts/release_lock_spec.rb diff --git a/lib/sidekiq_unique_jobs/scripts/release_lock.rb b/lib/sidekiq_unique_jobs/scripts/release_lock.rb index 3b5db1d78..4005575f4 100644 --- a/lib/sidekiq_unique_jobs/scripts/release_lock.rb +++ b/lib/sidekiq_unique_jobs/scripts/release_lock.rb @@ -10,10 +10,11 @@ def self.execute(redis_pool, unique_key, jid) attr_reader :redis_pool, :unique_key, :jid - def initialize(_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 @@ -38,7 +39,7 @@ def handle_result(result) logger.debug { "#{unique_key} is not a known key" } false else - raise "#{calling_method} returned an unexpected value (#{result})" + raise UnexpectedValue, "failed to release lock : unexpected return value (#{result})" 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 new file mode 100644 index 000000000..0fb8b499e --- /dev/null +++ b/spec/lib/sidekiq_unique_jobs/scripts/release_lock_spec.rb @@ -0,0 +1,41 @@ +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 From 76cc3c891b1aeafaf9739de41a88e3f94c8eaadf Mon Sep 17 00:00:00 2001 From: Mikael Henriksson Date: Sun, 16 Apr 2017 19:30:29 +0200 Subject: [PATCH 4/7] Remove bundle_audit --- .codeclimate.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.codeclimate.yml b/.codeclimate.yml index 1978b5039..7f6a9ab4e 100644 --- a/.codeclimate.yml +++ b/.codeclimate.yml @@ -1,7 +1,7 @@ --- engines: bundler-audit: - enabled: true + enabled: false fixme: enabled: true duplication: From b08e48088e53b1ab4f74465f0e416f0dd427a000 Mon Sep 17 00:00:00 2001 From: Mikael Henriksson Date: Sun, 16 Apr 2017 19:34:29 +0200 Subject: [PATCH 5/7] Sort keys alphabetically --- lib/sidekiq_unique_jobs/cli.rb | 2 +- spec/lib/sidekiq_unique_jobs/cli_spec.rb | 11 ++++++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/sidekiq_unique_jobs/cli.rb b/lib/sidekiq_unique_jobs/cli.rb index d4280fbd9..3104fb632 100644 --- a/lib/sidekiq_unique_jobs/cli.rb +++ b/lib/sidekiq_unique_jobs/cli.rb @@ -15,7 +15,7 @@ def self.banner(command, _namespace = nil, _subcommand = false) def keys(pattern) keys = Util.keys(pattern, options[:count]) say "Found #{keys.size} keys matching '#{pattern}':" - print_in_columns(keys) if keys.any? + print_in_columns(keys.sort) if keys.any? end desc 'del PATTERN', 'deletes unique keys from redis by pattern' diff --git a/spec/lib/sidekiq_unique_jobs/cli_spec.rb b/spec/lib/sidekiq_unique_jobs/cli_spec.rb index 06041a0e1..cef27319f 100644 --- a/spec/lib/sidekiq_unique_jobs/cli_spec.rb +++ b/spec/lib/sidekiq_unique_jobs/cli_spec.rb @@ -92,7 +92,7 @@ end context 'when a key exists' do - let(:keys) { ['defghayl', jid, 'poilkij'] } + let(:keys) { ['defghayl', jid, 'poilkij'].sort } before do keys.each do |jid| unique_key = "uniquejobs:#{jid}" @@ -107,9 +107,14 @@ after { SidekiqUniqueJobs::Util.del('*', 1000, false) } let(:expected) do - "Found 3 keys matching '#{pattern}':\nuniquejobs:poilkij uniquejobs:abcdefab uniquejobs:defghayl\n" + <<~EOS + Found 3 keys matching '#{pattern}': + uniquejobs:abcdefab uniquejobs:defghayl uniquejobs:poilkij + EOS + end + specify do + expect(output).to eq(expected) end - specify { expect(output).to eq(expected) } end end From 99aae9418e9134133f9250b2b1e11e92debed8e1 Mon Sep 17 00:00:00 2001 From: Mikael Henriksson Date: Sun, 16 Apr 2017 19:43:41 +0200 Subject: [PATCH 6/7] Stop doing gem system updated [ci skip] --- .travis.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.travis.yml b/.travis.yml index c280e8818..c7315d987 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,10 +1,10 @@ sudo: false language: ruby cache: bundler -before_install: - - rvm get head - - gem update --system - - gem install bundler +# before_install: +# - rvm get head +# - gem update --system +# - gem install bundler services: - redis-server script: From 7b0ce8a742b431845c06b2bd0a2c5c32957fe583 Mon Sep 17 00:00:00 2001 From: Mikael Henriksson Date: Sun, 16 Apr 2017 19:49:42 +0200 Subject: [PATCH 7/7] Don't use <<~ for ruby less than 2.4 --- spec/lib/sidekiq_unique_jobs/cli_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/lib/sidekiq_unique_jobs/cli_spec.rb b/spec/lib/sidekiq_unique_jobs/cli_spec.rb index cef27319f..63130a79c 100644 --- a/spec/lib/sidekiq_unique_jobs/cli_spec.rb +++ b/spec/lib/sidekiq_unique_jobs/cli_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' require 'thor/runner' -RSpec.describe SidekiqUniqueJobs::Cli do +RSpec.describe SidekiqUniqueJobs::Cli, ruby_ver: '>= 2.4' do describe '#help' do let(:output) { capture(:stdout) { described_class.start(%w[help]) } } let(:banner) do