Fix bug where expiration wasn't set until unlock (#481)

* Begin adding coverage for keys and expiration * Adds coverage for created/removed/updates keys Also fixes a bug that was accidentally introduced somewhere where expiration was not set for when locking anything except until_expired. * Remove rspec eventually * Make tests less specific to reduce complexity Now that the various lua files are better tested there is less need to test specifics in other places.
mhenrixon · Mar 25, 2020 · c6e3ff1 · c6e3ff1
1 parent 8c555f5
commit c6e3ff1
Show file tree

Hide file tree

Showing 27 changed files with 345 additions and 103 deletions.
diff --git a/Gemfile b/Gemfile
@@ -5,9 +5,8 @@ gemspec
 
 LOCAL_GEMS = "Gemfile.local"
 
-gem "appraisal",        ">= 2.2"
-gem "rspec-eventually", require: false
-gem "rspec-its",        require: false
+gem "appraisal", ">= 2.2"
+gem "rspec-its", require: false
 
 platforms :mri do
   gem "fasterer"

diff --git a/gemfiles/sidekiq_4.0.gemfile b/gemfiles/sidekiq_4.0.gemfile
@@ -3,7 +3,6 @@
 source "https://rubygems.org"
 
 gem "appraisal", ">= 2.2"
-gem "rspec-eventually", require: false
 gem "rspec-its", require: false
 gem "sidekiq", "~> 4.0.0"
 

diff --git a/gemfiles/sidekiq_4.1.gemfile b/gemfiles/sidekiq_4.1.gemfile
@@ -3,7 +3,6 @@
 source "https://rubygems.org"
 
 gem "appraisal", ">= 2.2"
-gem "rspec-eventually", require: false
 gem "rspec-its", require: false
 gem "sidekiq", "~> 4.1.0"
 

diff --git a/gemfiles/sidekiq_4.2.gemfile b/gemfiles/sidekiq_4.2.gemfile
@@ -3,7 +3,6 @@
 source "https://rubygems.org"
 
 gem "appraisal", ">= 2.2"
-gem "rspec-eventually", require: false
 gem "rspec-its", require: false
 gem "sidekiq", "~> 4.2.0"
 

diff --git a/gemfiles/sidekiq_5.0.gemfile b/gemfiles/sidekiq_5.0.gemfile
@@ -3,7 +3,6 @@
 source "https://rubygems.org"
 
 gem "appraisal", ">= 2.2"
-gem "rspec-eventually", require: false
 gem "rspec-its", require: false
 gem "sidekiq", "~> 5.0.0"
 

diff --git a/gemfiles/sidekiq_5.1.gemfile b/gemfiles/sidekiq_5.1.gemfile
@@ -3,7 +3,6 @@
 source "https://rubygems.org"
 
 gem "appraisal", ">= 2.2"
-gem "rspec-eventually", require: false
 gem "rspec-its", require: false
 gem "sidekiq", "~> 5.1.0"
 

diff --git a/gemfiles/sidekiq_5.2.gemfile b/gemfiles/sidekiq_5.2.gemfile
@@ -3,7 +3,6 @@
 source "https://rubygems.org"
 
 gem "appraisal", ">= 2.2"
-gem "rspec-eventually", require: false
 gem "rspec-its", require: false
 gem "sidekiq", "~> 5.2.0"
 

diff --git a/gemfiles/sidekiq_6.0.gemfile b/gemfiles/sidekiq_6.0.gemfile
@@ -3,7 +3,6 @@
 source "https://rubygems.org"
 
 gem "appraisal", ">= 2.2"
-gem "rspec-eventually", require: false
 gem "rspec-its", require: false
 gem "sidekiq", ">= 6.0.pre", "< 6.1"
 

diff --git a/gemfiles/sidekiq_develop.gemfile b/gemfiles/sidekiq_develop.gemfile
@@ -3,7 +3,6 @@
 source "https://rubygems.org"
 
 gem "appraisal", ">= 2.2"
-gem "rspec-eventually", require: false
 gem "rspec-its", require: false
 gem "sidekiq", git: "https://github.com/mperham/sidekiq.git"
 

diff --git a/lib/sidekiq_unique_jobs/lua/lock.lua b/lib/sidekiq_unique_jobs/lua/lock.lua
@@ -8,13 +8,15 @@ local changelog = KEYS[6]
 local digests   = KEYS[7]
 -------- END keys ---------
 
+
 -------- BEGIN lock arguments ---------
 local job_id       = ARGV[1]
 local pttl         = tonumber(ARGV[2])
 local lock_type    = ARGV[3]
 local limit        = tonumber(ARGV[4])
 -------- END lock arguments -----------
 
+
 --------  BEGIN injected arguments --------
 local current_time = tonumber(ARGV[5])
 local debug_lua    = ARGV[6] == "true"
@@ -23,6 +25,7 @@ local script_name  = tostring(ARGV[8]) .. ".lua"
 local redisversion = ARGV[9]
 ---------  END injected arguments ---------
 
+
 --------  BEGIN local functions --------
 <%= include_partial "shared/_common.lua" %>
 ----------  END local functions ----------
@@ -68,7 +71,7 @@ redis.call("LREM", primed, 1, job_id)
 
 -- The Sidekiq client should only set pttl for until_expired
 -- The Sidekiq server should set pttl for all other jobs
-if lock_type == "until_expired" and pttl and pttl > 0 then
+if pttl and pttl > 0 then
   log_debug("PEXPIRE", digest, pttl)
   redis.call("PEXPIRE", digest, pttl)
 

diff --git a/lib/sidekiq_unique_jobs/lua/queue.lua b/lib/sidekiq_unique_jobs/lua/queue.lua
@@ -8,27 +8,31 @@ local changelog = KEYS[6]
 local digests   = KEYS[7]
 -------- END keys ---------
 
+
 -------- BEGIN lock arguments ---------
-local job_id       = ARGV[1]      -- The job_id that was previously primed
-local pttl         = tonumber(ARGV[2])
-local lock_type    = ARGV[3]
-local limit        = tonumber(ARGV[4])
+local job_id    = ARGV[1]      -- The job_id that was previously primed
+local pttl      = tonumber(ARGV[2])
+local lock_type = ARGV[3]
+local limit     = tonumber(ARGV[4])
 -------- END lock arguments -----------
 
+
 --------  BEGIN injected arguments --------
 local current_time = tonumber(ARGV[5])
 local debug_lua    = ARGV[6] == "true"
 local max_history  = tonumber(ARGV[7])
 local script_name  = tostring(ARGV[8]) .. ".lua"
 ---------  END injected arguments ---------
 
+
 --------  BEGIN Variables --------
 local queued_count = redis.call("LLEN", queued)
 local locked_count = redis.call("HLEN", locked)
 local within_limit = limit > locked_count
 local limit_exceeded = not within_limit
 --------   END Variables  --------
 
+
 --------  BEGIN local functions --------
 <%= include_partial "shared/_common.lua" %>
 ----------  END local functions ----------
@@ -70,7 +74,7 @@ redis.call("LPUSH", queued, job_id)
 
 -- The Sidekiq client should only set pttl for until_expired
 -- The Sidekiq server should set pttl for all other jobs
-if lock_type == "until_expired" and pttl > 0 then
+if pttl and pttl > 0 then
   log_debug("PEXPIRE", digest, pttl)
   redis.call("PEXPIRE", digest, pttl)
   log_debug("PEXPIRE", queued, pttl)

diff --git a/lib/sidekiq_unique_jobs/lua/unlock.lua b/lib/sidekiq_unique_jobs/lua/unlock.lua
@@ -8,13 +8,15 @@ local changelog = KEYS[6]
 local digests   = KEYS[7]
 -------- END keys ---------
 
+
 -------- BEGIN lock arguments ---------
-local job_id = ARGV[1]
-local pttl   = tonumber(ARGV[2])
-local type   = ARGV[3]
-local limit  = tonumber(ARGV[4])
+local job_id    = ARGV[1]
+local pttl      = tonumber(ARGV[2])
+local lock_type = ARGV[3]
+local limit     = tonumber(ARGV[4])
 -------- END lock arguments -----------
 
+
 --------  BEGIN injected arguments --------
 local current_time = tonumber(ARGV[5])
 local debug_lua    = ARGV[6] == "true"
@@ -23,12 +25,14 @@ local script_name  = tostring(ARGV[8]) .. ".lua"
 local redisversion = ARGV[9]
 ---------  END injected arguments ---------
 
+
 --------  BEGIN Variables --------
 local queued_count = redis.call("LLEN", queued)
 local primed_count = redis.call("LLEN", primed)
 local locked_count = redis.call("HLEN", locked)
 ---------  END Variables ---------
 
+
 --------  BEGIN local functions --------
 <%= include_partial "shared/_common.lua" %>
 ----------  END local functions ----------
@@ -65,21 +69,12 @@ redis.call("LREM", primed, -1, job_id)
 log_debug("ZREM", digests, digest)
 redis.call("ZREM", digests, digest)
 
-if pttl and pttl > 0 then
-  log_debug("PEXPIRE", digest, pttl)
-  redis.call("PEXPIRE", digest, pttl)
-
-  log_debug("PEXPIRE", locked, pttl)
-  redis.call("PEXPIRE", locked, pttl)
-
-  log_debug("PEXPIRE", info, pttl)
-  redis.call("PEXPIRE", info, pttl)
-else
-  local redis_version = toversion(redisversion)
-  local del_cmd       = "DEL"
+local redis_version = toversion(redisversion)
+local del_cmd       = "DEL"
 
-  if tonumber(redis_version["major"]) >= 4 then del_cmd =  "UNLINK"; end
+if tonumber(redis_version["major"]) >= 4 then del_cmd =  "UNLINK"; end
 
+if lock_type ~= "until_expired" then
   log_debug(del_cmd, digest, info)
   redis.call(del_cmd, digest, info)
 
@@ -90,8 +85,8 @@ end
 log_debug("LPUSH", queued, "1")
 redis.call("LPUSH", queued, "1")
 
-log_debug("PEXPIRE", queued, 500)
-redis.call("PEXPIRE", queued, 500)
+log_debug("PEXPIRE", queued, 5000)
+redis.call("PEXPIRE", queued, 5000)
 
 log("Unlocked")
 log_debug("END unlock digest:", digest, "(job_id: " .. job_id ..")")

diff --git a/lib/sidekiq_unique_jobs/unlockable.rb b/lib/sidekiq_unique_jobs/unlockable.rb
@@ -11,6 +11,13 @@ module Unlockable
     # Unlocks a job.
     # @param [Hash] item a Sidekiq job hash
     def unlock(item)
+      SidekiqUniqueJobs::Job.add_digest(item)
+      SidekiqUniqueJobs::Locksmith.new(item).unlock
+    end
+
+    # Unlocks a job.
+    # @param [Hash] item a Sidekiq job hash
+    def unlock!(item)
       SidekiqUniqueJobs::Job.add_digest(item)
       SidekiqUniqueJobs::Locksmith.new(item).unlock!
     end

diff --git a/lib/sidekiq_unique_jobs/version.rb b/lib/sidekiq_unique_jobs/version.rb
@@ -3,5 +3,5 @@
 module SidekiqUniqueJobs
   #
   # @return [String] the current SidekiqUniqueJobs version
-  VERSION = "7.0.0.beta11.1"
+  VERSION = "7.0.0.beta12"
 end
diff --git a/myapp/Gemfile b/myapp/Gemfile
@@ -11,7 +11,7 @@ gem "json"
 gem "pg"
 gem "puma"
 gem "rack-protection"
-gem "rails", ">= 5.2"
+gem "rails", ">= 6.0"
 gem "redis"
 gem "sidekiq", "~> 6.0.0"
 gem "sidekiq-cron"
@@ -29,7 +29,6 @@ group :development, :test do
   gem "listen"
   gem "pry-byebug"
   gem "pry-rails"
-  gem "rspec-eventually"
   gem "rspec-rails"
   gem "rubocop"
   gem "rubocop-performance"

diff --git a/sidekiq-unique-jobs.gemspec b/sidekiq-unique-jobs.gemspec
@@ -51,13 +51,13 @@ Gem::Specification.new do |spec|
   spec.add_development_dependency "rake", "~> 13.0"
   spec.add_development_dependency "rspec", "~> 3.9"
   spec.add_development_dependency "sinatra", ">= 2.0", "< 3.0"
-  spec.add_development_dependency "timecop" #, "~> 0.9"
+  spec.add_development_dependency "timecop", "~> 0.9"
 
   # ===== Documentation =====
-  spec.add_development_dependency "github-markup" #, "~> 3.0"
-  spec.add_development_dependency "github_changelog_generator" #, "~> 1.14"
-  spec.add_development_dependency "yard" #, "~> 0.9.18"
+  spec.add_development_dependency "github-markup", "~> 3.0"
+  spec.add_development_dependency "github_changelog_generator", "~> 1.14"
+  spec.add_development_dependency "yard", "~> 0.9.18"
 
   # ===== Release Management =====
-  spec.add_development_dependency "gem-release" #, "~> 2.0"
+  spec.add_development_dependency "gem-release", "~> 2.0"
 end
diff --git a/spec/sidekiq_unique_jobs/lock/until_executed_spec.rb b/spec/sidekiq_unique_jobs/lock/until_executed_spec.rb
@@ -42,7 +42,7 @@
       process_one.execute do
         expect(process_one).to be_locked
       end
-      expect(process_one).to be_locked # Because we have expiration set to 5000
+      expect(process_one).not_to be_locked # Because we have expiration set to 5000
     end
   end
 

diff --git a/spec/sidekiq_unique_jobs/lock/until_executing_spec.rb b/spec/sidekiq_unique_jobs/lock/until_executing_spec.rb
@@ -60,7 +60,7 @@
         it "does not delete the lock" do
           worker_class.use_options(lock_ttl: 1000) do
             process_one.lock
-            expect { unique_keys.size }.to eventually eq(3)
+            expect(unique_keys.size).to eq(3)
             expect { delete }.not_to change { unique_keys.size }
           end
 

diff --git a/spec/sidekiq_unique_jobs/locksmith_spec.rb b/spec/sidekiq_unique_jobs/locksmith_spec.rb
@@ -192,10 +192,10 @@
         locksmith_one.lock
         expect(locksmith_one).to be_locked
         locksmith_one.unlock
-        expect(locksmith_one).to be_locked
+        expect(locksmith_one).not_to be_locked
         expect(locksmith_one.delete).to eq(nil)
 
-        expect(locksmith_one).to be_locked
+        expect(locksmith_one).not_to be_locked
       end
     end
 
@@ -217,7 +217,7 @@
         # noop
       end
       keys = unique_keys
-      expect { unique_keys }.to eventually_not include(keys)
+      expect(unique_keys).not_to include(keys)
     end
   end