From 6b140101992dc0059754e6290740cf8e64ba657d Mon Sep 17 00:00:00 2001 From: Samwise Wang Date: Fri, 30 Sep 2016 17:31:08 +0800 Subject: [PATCH 1/2] Fix the error in worker#perform_in about interval type (#3170) Situation: We are using Sidekiq Pro with ActiveSupport. When I passed a object which is ActiveSupport::TimeWithZone to perform_at, `TypeError: can't convert ActiveSupport::TimeWithZone into an exact number` has occurred. Problem: Time can't plus a ActiveSupport::TimeWithZone object. Solution: We can transform any Time object to float, and use it to compare and calculate. --- lib/sidekiq/worker.rb | 6 +++--- test/test_scheduling.rb | 8 ++++++++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/lib/sidekiq/worker.rb b/lib/sidekiq/worker.rb index 18b2ecb7..df8d6c51 100644 --- a/lib/sidekiq/worker.rb +++ b/lib/sidekiq/worker.rb @@ -64,13 +64,13 @@ module Sidekiq # numeric (like an activesupport time interval). def perform_in(interval, *args) int = interval.to_f - now = Time.now - ts = (int < 1_000_000_000 ? (now + interval).to_f : int) + now = Time.now.to_f + ts = (int < 1_000_000_000 ? now + int : int) item = { 'class' => self, 'args' => args, 'at' => ts } # Optimization to enqueue something now that is scheduled to go out now or in the past - item.delete('at'.freeze) if ts <= now.to_f + item.delete('at'.freeze) if ts <= now client_push(item) end diff --git a/test/test_scheduling.rb b/test/test_scheduling.rb index 0d3e4dc6..f429471b 100644 --- a/test/test_scheduling.rb +++ b/test/test_scheduling.rb @@ -11,6 +11,11 @@ class TestScheduling < Sidekiq::Test end end + # Assume we can pass any class as time to perform_in + class TimeDuck + def to_f; 42.0 end + end + it 'schedules jobs' do ss = Sidekiq::ScheduledSet.new ss.clear @@ -34,6 +39,9 @@ class TestScheduling < Sidekiq::Test assert Sidekiq::Client.push_bulk('class' => ScheduledWorker, 'args' => [['mike'], ['mike']], 'at' => 600) assert_equal 5, ss.size + + assert ScheduledWorker.perform_in(TimeDuck.new, 'samwise') + assert_equal 6, ss.size end it 'removes the enqueued_at field when scheduling' do From 659dea9601b3a283d9c85eee1ca3cf11e967c660 Mon Sep 17 00:00:00 2001 From: Samuel Cochran Date: Sat, 1 Oct 2016 08:27:54 +1000 Subject: [PATCH 2/2] Resolve Rails Reloader and ActiveRecord middleware incompatibility (#3166) * Prevent AR middleware and reloader clash The ActiveRecord middleware releases any current ActiveRecord connections before the reloader has a chance to clear their query caches. We can just remove this middleware because the reloader takes care of clearing active connections, too. * Make sure folks haven't added the middleware manually * Fix the conditional * Mention that the reloader also clears connections * Move it all back into a single method * Typos * Pull all rails setup into railtie * Set the middleware even earlier Also clarify exactly when these hooks are run. * Only check ActiveRecord reloader/middleware sanity at boot --- lib/sidekiq.rb | 4 ---- lib/sidekiq/cli.rb | 1 - lib/sidekiq/rails.rb | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 33 insertions(+), 5 deletions(-) diff --git a/lib/sidekiq.rb b/lib/sidekiq.rb index 188e2d82..51c551b3 100644 --- a/lib/sidekiq.rb +++ b/lib/sidekiq.rb @@ -150,10 +150,6 @@ module Sidekiq Middleware::Chain.new do |m| m.add Middleware::Server::Logging m.add Middleware::Server::RetryJobs - if defined?(::ActiveRecord::Base) - require 'sidekiq/middleware/server/active_record' - m.add Sidekiq::Middleware::Server::ActiveRecord - end end end diff --git a/lib/sidekiq/cli.rb b/lib/sidekiq/cli.rb index 6a1b1033..66ba2033 100644 --- a/lib/sidekiq/cli.rb +++ b/lib/sidekiq/cli.rb @@ -240,7 +240,6 @@ module Sidekiq else require 'sidekiq/rails' require File.expand_path("#{options[:require]}/config/environment.rb") - Sidekiq.options[:reloader] = Sidekiq::Rails::Reloader.new end options[:tag] ||= default_tag else diff --git a/lib/sidekiq/rails.rb b/lib/sidekiq/rails.rb index 23b27376..a5ddc5ed 100644 --- a/lib/sidekiq/rails.rb +++ b/lib/sidekiq/rails.rb @@ -32,10 +32,43 @@ module Sidekiq end class Rails < ::Rails::Engine + # We need to setup this up before any application configuration which might + # change Sidekiq middleware. + # + # This hook happens after `Rails::Application` is inherited within + # config/application.rb and before config is touched, usually within the + # class block. Definitely before config/environments/*.rb and + # config/initializers/*.rb. + config.before_configuration do + if ::Rails::VERSION::MAJOR < 5 && defined?(::ActiveRecord) + Sidekiq.server_middleware do |chain| + require 'sidekiq/middleware/server/active_record' + chain.add Sidekiq::Middleware::Server::ActiveRecord + end + end + end + initializer 'sidekiq' do Sidekiq.hook_rails! end + # We have to add the reloader after initialize to see if cache_classes has + # been turned on. + # + # This hook happens after all initialziers are run, just before returning + # from config/environment.rb back to sidekiq/cli.rb. + config.after_initialize do + if ::Rails::VERSION::MAJOR >= 5 + # The reloader also takes care of ActiveRecord but is incompatible with + # the ActiveRecord middleware so make sure it's not in the chain already. + if defined?(Sidekiq::Middleware::Server::ActiveRecord) && Sidekiq.server_middleware.exists?(Sidekiq::Middleware::Server::ActiveRecord) + raise ArgumentError, "You are using the Sidekiq ActiveRecord middleware and the new Rails 5 reloader which are incompatible. Please remove the ActiveRecord middleware from your Sidekiq middleware configuration." + else + Sidekiq.options[:reloader] = Sidekiq::Rails::Reloader.new + end + end + end + class Reloader def initialize(app = ::Rails.application) Sidekiq.logger.debug "Enabling Rails 5+ live code reloading, so hot!" unless app.config.cache_classes