Skip to content

Commit

Permalink
Use Rails executor if code reloading is disabled (#3221)
Browse files Browse the repository at this point in the history
We have to release the current thread's Active Record connection after
performing each job, in case another thread is waiting to use it.

On Rails 4 and earlier, this is handled with middleware. On Rails 5 in
development mode, the reloader does it by delegating to the executor.
However on Rails 5 in production mode, we're not adding the middleware
or enabling the reloader, so connections will never be released.

We can call the executor directly to have it release the connection for
us in this case. By calling it inside the middleware stack, the job will
be retried if the executor raises, avoiding the problem with lost jobs
that led to the reloader being disabled in production.
  • Loading branch information
eugeneius authored and mperham committed Nov 12, 2016
1 parent ee4bf1d commit f49b4f1
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 6 deletions.
1 change: 1 addition & 0 deletions lib/sidekiq.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ module Sidekiq
dead_max_jobs: 10_000,
dead_timeout_in_seconds: 180 * 24 * 60 * 60, # 6 months
reloader: proc { |&block| block.call },
executor: proc { |&block| block.call },
}

DEFAULT_WORKER_OPTIONS = {
Expand Down
13 changes: 8 additions & 5 deletions lib/sidekiq/processor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ def initialize(mgr)
@thread = nil
@strategy = (mgr.options[:fetch] || Sidekiq::BasicFetch).new(mgr.options)
@reloader = Sidekiq.options[:reloader]
@executor = Sidekiq.options[:executor]
end

def terminate(wait=false)
Expand Down Expand Up @@ -129,11 +130,13 @@ def process(work)

stats(worker, job_hash, queue) do
Sidekiq.server_middleware.invoke(worker, job_hash, queue) do
# Only ack if we either attempted to start this job or
# successfully completed it. This prevents us from
# losing jobs if a middleware raises an exception before yielding
ack = true
execute_job(worker, cloned(job_hash['args'.freeze]))
@executor.call do
# Only ack if we either attempted to start this job or
# successfully completed it. This prevents us from
# losing jobs if a middleware raises an exception before yielding
ack = true
execute_job(worker, cloned(job_hash['args'.freeze]))
end
end
end
ack = true
Expand Down
19 changes: 18 additions & 1 deletion lib/sidekiq/rails.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,15 +67,32 @@ class Rails < ::Rails::Engine
# The reloader API has proven to be troublesome under load in production.
# We won't use it at all when classes are cached, see #3154
Sidekiq.logger.debug { "Autoload disabled in #{::Rails.env}, Sidekiq will not reload changed classes" }
Sidekiq.options[:executor] = Sidekiq::Rails::Executor.new
else
Sidekiq.logger.debug { "Enabling Rails 5+ live code reloading, so hot!" }
Sidekiq.options[:reloader] = Sidekiq::Rails::Reloader.new
end
end
end

class Executor
def initialize(app = ::Rails.application)
@app = app
end

def call
@app.executor.wrap do
yield
end
end

def inspect
"#<Sidekiq::Rails::Executor @app=#{@app.class.name}>"
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
@app = app
end

Expand Down

0 comments on commit f49b4f1

Please sign in to comment.