fix(unlock): ensure callback and unlock (mhenrixon#771)

* chore(deps): update gems (solargraph is awesome)

* fix(unlock): ensure unlock and callback runs

* chore(lint): lint'em real good

Author: mhenrixon

Diff for: .github/workflows/lint.yml

@@ -14,7 +14,7 @@ jobs:
       - uses: ruby/setup-ruby@v1
         with:
           ruby-version: 3.1
-          bundler: 2.3.19
+          bundler: 2.4.12
           bundler-cache: true
       - run: bin/bundle --jobs=$(nproc) --retry=$(nproc)
       - run: bin/rubocop -P
@@ -29,7 +29,7 @@ jobs:
       - uses: ruby/setup-ruby@v1
         with:
           ruby-version: 3.1
-          bundler: 2.3.19
+          bundler: 2.4.12
           bundler-cache: true
       - run: bin/bundle --jobs=$(nproc) --retry=$(nproc)
       - run: bin/reek .

Diff for: .github/workflows/rspec.yml

@@ -23,7 +23,7 @@ jobs:
       - uses: ruby/setup-ruby@v1
         with:
           ruby-version: 3.2
-          bundler: 2.4.2
+          bundler: 2.4.12
           bundler-cache: true
 
       - name: Install Code Climate reporter
@@ -59,14 +59,15 @@ jobs:
     strategy:
       fail-fast: true
       matrix:
-        ruby: [2.7, '3.0', 3.1]
+        ruby: [2.7, '3.0', 3.1, 3.2]
 
     steps:
       - uses: actions/checkout@v3
       - uses: ruby/setup-ruby@v1
         with:
           ruby-version: ${{ matrix.ruby }}
-          bundler: 2.4.2
+          bundler: 2.4.12
           bundler-cache: true
+      - run: bundle install --gemfile='./gemfiles/sidekiq_7.0.gemfile' --retry 1
       - run: bin/appraisal install --jobs=$(nproc) --retry=$(nproc)
       - run: bin/appraisal rspec --require spec_helper --tag ~perf

Diff for: Gemfile

@@ -31,12 +31,10 @@ end
 
 if respond_to?(:install_if)
   install_if -> { RUBY_PLATFORM.include?("darwin") } do
-    gem "fasterer"
     gem "fuubar"
     gem "github_changelog_generator"
     gem "pry"
     gem "redcarpet", "~> 3.4"
-    gem "rspec-nc"
     gem "ruby-prof", ">= 0.17.0", require: false
     gem "stackprof", ">= 0.2.9", require: false
     gem "test-prof"

Diff for: bin/bundle

@@ -63,9 +63,11 @@ m = Module.new do
   end
 
   def bundler_version
+    # rubocop:disable ThreadSafety/InstanceVariableInClassMethod
     @bundler_version ||=
       env_var_version || cli_arg_version ||
       lockfile_version
+    # rubocop:enable ThreadSafety/InstanceVariableInClassMethod
   end
 
   def bundler_requirement

Diff for: gemfiles/sidekiq_7.0.gemfile

@@ -3,11 +3,11 @@
 source "https://rubygems.org"
 
 gem "appraisal"
+gem "faraday-retry"
 gem "gem-release"
 gem "github-markup"
 gem "rack-test"
 gem "rake", "13.0.3"
-gem "redis-namespace"
 gem "reek", ">= 5.3"
 gem "rspec"
 gem "rspec-benchmark"

Diff for: lib/sidekiq_unique_jobs/lock/until_executed.rb

@@ -35,6 +35,7 @@ def lock(&block)
       def execute
         executed = locksmith.execute do
           yield
+        ensure
           unlock_and_callback
         end
 

Diff for: lib/sidekiq_unique_jobs/lock/while_executing.rb

@@ -42,9 +42,8 @@ def execute(&block)
         with_logging_context do
           executed = locksmith.execute do
             yield
-            callback_safely if locksmith.unlock
           ensure
-            locksmith.unlock
+            unlock_and_callback
           end
 
           unless executed

Diff for: lib/sidekiq_unique_jobs/orphans/manager.rb

@@ -72,7 +72,7 @@ def stop
       # @return [<type>] <description>
       #
       def task
-        @task ||= default_task
+        @task ||= default_task # rubocop:disable ThreadSafety/InstanceVariableInClassMethod
       end
 
       #
@@ -100,7 +100,7 @@ def default_task
       # @return [void]
       #
       def task=(task)
-        @task = task
+        @task = task # rubocop:disable ThreadSafety/InstanceVariableInClassMethod
       end
 
       #

Diff for: lib/sidekiq_unique_jobs/sidekiq_unique_jobs.rb

@@ -17,7 +17,7 @@ module SidekiqUniqueJobs # rubocop:disable Metrics/ModuleLength
   # @return [SidekiqUniqueJobs::Config] the gem configuration
   #
   def config
-    @config ||= reset!
+    @config ||= reset! # rubocop:disable ThreadSafety/InstanceVariableInClassMethod
   end
 
   #
@@ -108,7 +108,7 @@ def use_config(tmp_config = {})
   # @return [SidekiqUniqueJobs::Config] a default gem configuration
   #
   def reset!
-    @config = SidekiqUniqueJobs::Config.default
+    @config = SidekiqUniqueJobs::Config.default # rubocop:disable ThreadSafety/InstanceVariableInClassMethod
   end
 
   #
@@ -288,7 +288,7 @@ def safe_constantize(str)
   # @return [Reflections]
   #
   def reflections
-    @reflections ||= Reflections.new
+    @reflections ||= Reflections.new # rubocop:disable ThreadSafety/InstanceVariableInClassMethod
   end
 
   #

Diff for: myapp/.tool-versions

@@ -1 +1,4 @@
-ruby 3.2.0
+ruby 3.2.2
+nodejs 19.1.0
+yarn 1.22.19
+direnv 2.32.2

Diff for: spec/sidekiq_unique_jobs/lock/while_executing_spec.rb

@@ -77,27 +77,33 @@
     context "when no callback is defined" do
       let(:job_class) { WhileExecutingRescheduleJob }
       let(:callback_one) { -> { true } }
-      let(:callback_two) { nil }
+      let(:callback_two) { -> { true } }
 
       let(:strategy_one) { process_one.send(:server_strategy) }
       let(:strategy_two) { process_two.send(:server_strategy) }
 
       before do
         allow(strategy_one).to receive(:call).and_call_original
         allow(strategy_two).to receive(:call).and_call_original
+        allow(process_one).to receive(:reflect).and_call_original
         allow(process_two).to receive(:reflect).and_call_original
       end
 
       it "reflects execution_failed" do
         process_one.execute do
           process_two.execute { puts "BOGUS!" }
+          # NOTE: Below looks weird but tests that
+          #   the result from process_two (which is nil) isn't considered.
+          jid_one
         end
 
+        expect(process_one).not_to have_received(:reflect).with(:execution_failed, item_one)
         expect(callback_one).to have_received(:call).once
         expect(strategy_one).not_to have_received(:call)
-        expect(strategy_two).to have_received(:call).once
 
         expect(process_two).to have_received(:reflect).with(:execution_failed, item_two)
+        expect(callback_two).not_to have_received(:call)
+        expect(strategy_two).to have_received(:call).once
       end
     end
 

Diff for: spec/support/shared_examples/a_lockable_lock.rb

@@ -35,61 +35,3 @@
     end
   end
 end
-
-RSpec.shared_examples "an executing lock implementation" do
-  context "when job can't be locked" do
-    before do
-      allow(process_one.locksmith).to receive(:execute).and_return(nil)
-    end
-
-    it "does not execute" do
-      unset = true
-      process_one.execute { unset = false }
-      expect(unset).to be(true)
-    end
-  end
-
-  context "when process_one executes the job" do
-    before { process_one.lock }
-
-    it "keeps being locked while executing" do
-      process_one.execute do
-        expect(process_one).to be_locked
-      end
-    end
-
-    it "keeps being locked when an error is raised" do
-      allow(process_one.locksmith).to receive(:execute).and_raise(RuntimeError, "Hell")
-
-      expect { process_one.execute { "hey ho" } }.to raise_error("Hell")
-
-      expect(process_one).to be_locked
-    end
-
-    it "prevents process_two from locking" do
-      process_one.execute do
-        expect(process_two.lock).to be_nil
-        expect(process_two).not_to be_locked
-      end
-    end
-
-    it "prevents process_two from executing" do
-      expect { process_two.execute { raise "Hell" } }.not_to raise_error
-    end
-
-    it "reflects execution_failed on failure" do
-      allow(process_two).to receive(:reflect).and_call_original
-      process_two.execute { puts "Failed to execute" }
-
-      expect(process_two).to have_received(:reflect).with(:execution_failed, item_two)
-    end
-
-    it "yields without arguments" do
-      process_one.lock
-      process_one.execute {}
-      blk = -> {}
-
-      expect { process_one.execute(&blk) }.not_to raise_error
-    end
-  end
-end

Diff for: spec/support/shared_examples/an_executing_lock_implementation.rb

@@ -0,0 +1,59 @@
+# frozen_string_literal: true
+
+RSpec.shared_examples "an executing lock implementation" do
+  context "when job can't be locked" do
+    before do
+      allow(process_one.locksmith).to receive(:execute).and_return(nil)
+    end
+
+    it "does not execute" do
+      unset = true
+      process_one.execute { unset = false }
+      expect(unset).to be(true)
+    end
+  end
+
+  context "when process_one executes the job" do
+    before { process_one.lock }
+
+    it "keeps being locked while executing" do
+      process_one.execute do
+        expect(process_one).to be_locked
+      end
+    end
+
+    it "keeps being locked when an error is raised" do
+      allow(process_one.locksmith).to receive(:execute).and_raise(RuntimeError, "Hell")
+
+      expect { process_one.execute { "hey ho" } }.to raise_error("Hell")
+
+      expect(process_one).to be_locked
+    end
+
+    it "prevents process_two from locking" do
+      process_one.execute do
+        expect(process_two.lock).to be_nil
+        expect(process_two).not_to be_locked
+      end
+    end
+
+    it "prevents process_two from executing" do
+      expect { process_two.execute { raise "Hell" } }.not_to raise_error
+    end
+
+    it "reflects execution_failed on failure" do
+      allow(process_two).to receive(:reflect).and_call_original
+      process_two.execute { puts "Failed to execute" }
+
+      expect(process_two).to have_received(:reflect).with(:execution_failed, item_two)
+    end
+
+    it "yields without arguments" do
+      process_one.lock
+      process_one.execute {}
+      blk = -> {}
+
+      expect { process_one.execute(&blk) }.not_to raise_error
+    end
+  end
+end