Give user full control over adding middleware (#566)

* Give user full control over adding middleware This prevents the middleware being added in the wrong place while removing some complexity from the gem. Making a decision on where to add the middleware became almost impossible. Give the user full controll, update the example configuration in the rails app and document all the known cases. * Configure the middleware for test * Remove specs for configuring middleware * Mandatory rubocop commit
mhenrixon · Jan 20, 2021 · b5565ba · b5565ba
1 parent a322b55
commit b5565ba
Show file tree

Hide file tree

Showing 8 changed files with 118 additions and 162 deletions.
diff --git a/README.md b/README.md
@@ -5,6 +5,7 @@
 - [Introduction](#introduction)
 - [Usage](#usage)
   - [Installation](#installation)
+  - [Add the middleware](#add-the-middleware)
   - [Your first worker](#your-first-worker)
 - [Support Me](#support-me)
 - [Requirements](#requirements)
@@ -49,7 +50,9 @@
   - [Logging](#logging)
   - [Cleanup Dead Locks](#cleanup-dead-locks)
   - [Other Sidekiq gems](#other-sidekiq-gems)
+    - [apartment-sidekiq](#apartment-sidekiq)
     - [sidekiq-global_id](#sidekiq-global_id)
+    - [sidekiq-status](#sidekiq-status)
 - [Debugging](#debugging)
   - [Sidekiq Web](#sidekiq-web)
     - [Show Locks](#show-locks)
@@ -91,6 +94,36 @@ And then execute:
 bundle
 ```
 
+### Add the middleware
+
+Before v7, the middleware was configured automatically. Since some people reported issues with other gems (see [Other Sidekiq Gems](#other-sidekiq-gems)) it was decided to give full control over to the user.
+
+[A full and hopefully working example](https://github.com/mhenrixon/sidekiq-unique-jobs/blob/master/myapp/config/sidekiq.rb#L12)
+
+```ruby
+Sidekiq.configure_server do |config|
+  config.redis = { url: ENV["REDIS_URL"], driver: :hiredis }
+
+  config.server_middleware do |chain|
+    chain.add SidekiqUniqueJobs::Middleware::Server
+  end
+
+  config.error_handlers << ->(ex, ctx_hash) { p ex, ctx_hash }
+  config.death_handlers << lambda do |job, _ex|
+    digest = job["lock_digest"]
+    SidekiqUniqueJobs::Digests.delete_by_digest(digest) if digest
+  end
+end
+
+Sidekiq.configure_client do |config|
+  config.redis = { url: ENV["REDIS_URL"], driver: :hiredis }
+
+  config.client_middleware do |chain|
+    chain.add SidekiqUniqueJobs::Middleware::Client
+  end
+end
+```
+
 ### Your first worker
 
 ```ruby
@@ -687,10 +720,30 @@ end
 
 ### Other Sidekiq gems
 
+#### apartment-sidekiq
+
+It was reported in [#536](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/536) that the order of the Sidekiq middleware needs to be as follows.
+
+```ruby
+Sidekiq.client_middleware do |chain|
+  chain.add Apartment::Sidekiq::Middleware::Client
+  chain.add SidekiqUniqueJobs::Middleware::Client
+end
+
+Sidekiq.server_middleware do |chain|
+  chain.add Apartment::Sidekiq::Middleware::Server
+  chain.add SidekiqUniqueJobs::Middleware::Server
+end
+```
+
+The reason being that this gem needs to be configured AFTER the apartment gem or the apartment will not be able to be considered for uniqueness
+
 #### sidekiq-global_id
 
 It was reported in [#235](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/235) that the order of the Sidekiq middleware needs to be as follows.
 
+For a working setup check the following [file](https://github.com/mhenrixon/sidekiq-unique-jobs/blob/master/myapp/config/sidekiq.rb#L12).
+
 ```ruby
 Sidekiq.client_middleware do |chain|
   chain.add Sidekiq::GlobalId::ClientMiddleware
@@ -703,7 +756,25 @@ Sidekiq.server_middleware do |chain|
 end
 ```
 
-For a working setup check the following [file](https://github.com/mhenrixon/sidekiq-unique-jobs/blob/945c4c4c517168d49e3f8ee952fcc9c430865635/myapp/config/initializers/sidekiq.rb#L8)
+The reason for this is that the global id needs to be set before the unique jobs middleware runs. Otherwise that won't be available for uniqueness.
+
+#### sidekiq-status
+
+It was reported in [#564](https://github.com/mhenrixon/sidekiq-unique-jobs/issues/564) that the order of the middleware needs to be as follows.
+
+```ruby
+Sidekiq.client_middleware do |chain|
+  chain.add Sidekiq::Status::ClientMiddleware, expiration: 10.minutes
+  chain.add Sidekiq::Status::ServerMiddleware
+end
+
+Sidekiq.server_middleware do |chain|
+  chain.add Sidekiq::Status::ServerMiddleware, expiration: 10.minutes
+  chain.add SidekiqUniqueJobs::Middleware::Server
+end
+```
+
+The reason for this is that if a job is duplicated it shouldn't end up with the status middleware at all. Status is just a monitor so to prevent clashes, leftovers and ensure cleanup. The status middleware should run after uniqueness on client and before on server. This will lead to less surprises.
 
 ## Debugging
 

diff --git a/lib/sidekiq-unique-jobs.rb b/lib/sidekiq-unique-jobs.rb
@@ -1,5 +1,3 @@
 # frozen_string_literal: true
 
 require "sidekiq_unique_jobs"
-
-SidekiqUniqueJobs::Middleware.configure
diff --git a/lib/sidekiq_unique_jobs/middleware.rb b/lib/sidekiq_unique_jobs/middleware.rb
@@ -11,63 +11,6 @@ module Middleware
     include SidekiqUniqueJobs::OptionsWithFallback
     include SidekiqUniqueJobs::JSON
 
-    #
-    # Configure both server and client
-    #
-    def self.configure
-      configure_server
-      configure_client
-    end
-
-    #
-    # Configures the Sidekiq server
-    #
-    def self.configure_server # rubocop:disable Metrics/MethodLength
-      Sidekiq.configure_server do |config|
-        config.client_middleware do |chain|
-          if defined?(Apartment::Sidekiq::Middleware::Client)
-            chain.insert_after Apartment::Sidekiq::Middleware::Client, SidekiqUniqueJobs::Middleware::Client
-          else
-            chain.add SidekiqUniqueJobs::Middleware::Client
-          end
-        end
-
-        config.server_middleware do |chain|
-          if defined?(Apartment::Sidekiq::Middleware::Server)
-            chain.insert_after Apartment::Sidekiq::Middleware::Server, SidekiqUniqueJobs::Middleware::Server
-          else
-            chain.add SidekiqUniqueJobs::Middleware::Server
-          end
-        end
-
-        config.on(:startup) do
-          SidekiqUniqueJobs::UpdateVersion.call
-          SidekiqUniqueJobs::UpgradeLocks.call
-
-          SidekiqUniqueJobs::Orphans::Manager.start
-        end
-
-        config.on(:shutdown) do
-          SidekiqUniqueJobs::Orphans::Manager.stop
-        end
-      end
-    end
-
-    #
-    # Configures the Sidekiq client
-    #
-    def self.configure_client
-      Sidekiq.configure_client do |config|
-        config.client_middleware do |chain|
-          if defined?(Apartment::Sidekiq::Middleware::Client)
-            chain.insert_after Apartment::Sidekiq::Middleware::Client, SidekiqUniqueJobs::Middleware::Client
-          else
-            chain.add SidekiqUniqueJobs::Middleware::Client
-          end
-        end
-      end
-    end
-
     # The sidekiq job hash
     # @return [Hash] the Sidekiq job hash
     attr_reader :item

diff --git a/myapp/Gemfile b/myapp/Gemfile
@@ -4,6 +4,7 @@ source "https://rubygems.org"
 
 ruby "2.7.2"
 
+gem "apartment-sidekiq"
 gem "bigdecimal"
 gem "coverband"
 gem "devise"

diff --git a/myapp/config/sidekiq.rb b/myapp/config/sidekiq.rb
@@ -9,42 +9,32 @@
   retry: true,
 }
 
-Sidekiq.client_middleware do |chain|
-  chain.add Sidekiq::GlobalId::ClientMiddleware
-  chain.add SidekiqUniqueJobs::Middleware::Client
-end
-
-Sidekiq.server_middleware do |chain|
-  chain.add SidekiqUniqueJobs::Middleware::Server
-  chain.add Sidekiq::GlobalId::ServerMiddleware
-end
-
 Sidekiq.configure_server do |config|
   config.redis = { url: ENV["REDIS_URL"], driver: :hiredis }
-  config.error_handlers << ->(ex, ctx_hash) { p ex, ctx_hash }
 
+  config.server_middleware do |chain|
+    chain.add Sidekiq::Status.ServerMiddleware, expiration: 30.minutes
+    chain.add Sidekiq::GlobalId::ServerMiddleware
+    chain.add Apartment::Sidekiq::Middleware::Server
+    chain.add SidekiqUniqueJobs::Middleware::Server
+  end
+
+  config.error_handlers << ->(ex, ctx_hash) { p ex, ctx_hash }
   config.death_handlers << lambda do |job, _ex|
     digest = job["lock_digest"]
     SidekiqUniqueJobs::Digests.delete_by_digest(digest) if digest
   end
-
-  # # accepts :expiration (optional)
-  Sidekiq::Status.configure_server_middleware config, expiration: 30.minutes
-
-  # # accepts :expiration (optional)
-  Sidekiq::Status.configure_client_middleware config, expiration: 30.minutes
-
-  # schedule_file = "config/schedule.yml"
-
-  # if File.exist?(schedule_file)
-  #   Sidekiq::Cron::Job.load_from_hash YAML.load_file(schedule_file)
-  # end
 end
 
 Sidekiq.configure_client do |config|
   config.redis = { url: ENV["REDIS_URL"], driver: :hiredis }
-  # accepts :expiration (optional)
-  Sidekiq::Status.configure_client_middleware config, expiration: 30.minutes
+
+  config.client_middleware do |chain|
+    chain.add Sidekiq::GlobalId::ClientMiddleware
+    chain.add Apartment::Sidekiq::Middleware::Client
+    chain.add SidekiqUniqueJobs::Middleware::Client
+    chain.add Sidekiq::Status.ClientMiddleware, expiration: 30.minutes
+  end
 end
 
 Sidekiq.logger       = Sidekiq::Logger.new($stdout)

diff --git a/sidekiq-unique-jobs.gemspec b/sidekiq-unique-jobs.gemspec
@@ -31,6 +31,9 @@ Gem::Specification.new do |spec|
   spec.post_install_message = <<~POST_INSTALL
     IMPORTANT!
 
+    Automatic configuration of the sidekiq middelware is no longer done.
+    Please see: https://github.com/mhenrixon/sidekiq-unique-jobs/blob/master/README.md#add-the-middleware
+
     This version deprecated the following sidekiq_options
 
       - sidekiq_options lock_args: :method_name

diff --git a/spec/sidekiq_unique_jobs/middleware_spec.rb b/spec/sidekiq_unique_jobs/middleware_spec.rb
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
@@ -27,6 +27,33 @@
 LOGLEVEL = ENV.fetch("LOGLEVEL", "ERROR").upcase
 ORIGINAL_SIDEKIQ_OPTIONS = Sidekiq.default_worker_options
 
+Sidekiq.default_worker_options = {
+  backtrace: true,
+  retry: true,
+}
+
+Sidekiq.configure_server do |config|
+  config.redis = { url: ENV["REDIS_URL"], driver: :hiredis }
+
+  config.server_middleware do |chain|
+    chain.add SidekiqUniqueJobs::Middleware::Server
+  end
+
+  config.error_handlers << ->(ex, ctx_hash) { p ex, ctx_hash }
+  config.death_handlers << lambda do |job, _ex|
+    digest = job["lock_digest"]
+    SidekiqUniqueJobs::Digests.delete_by_digest(digest) if digest
+  end
+end
+
+Sidekiq.configure_client do |config|
+  config.redis = { url: ENV["REDIS_URL"], driver: :hiredis }
+
+  config.client_middleware do |chain|
+    chain.add SidekiqUniqueJobs::Middleware::Client
+  end
+end
+
 SidekiqUniqueJobs.configure do |config|
   config.logger.level = Logger.const_get(LOGLEVEL)
   config.debug_lua    = %w[1 true].include?(ENV["DEBUG_LUA"])