Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable code reloading in development mode with Rails 5 #2457

Merged
merged 14 commits into from Aug 24, 2016
6 changes: 0 additions & 6 deletions .travis.yml
Expand Up @@ -7,12 +7,6 @@ before_install:
- gem install bundler
- gem update bundler
rvm:
- 2.0.0
- 2.1.8
- 2.2.4
- 2.3.0
- jruby-head
- rbx-2
matrix:
allow_failures:
- rvm: rbx-2
12 changes: 7 additions & 5 deletions Gemfile
@@ -1,7 +1,9 @@
source 'https://rubygems.org'
gemspec

gem 'rails', '~> 4.2'
gem 'rails', '5.0.0.beta2'
gem 'rack', '2.0.0.alpha'
gem 'sinatra', github: 'sinatra/sinatra'
gem 'simplecov'
gem 'minitest'
gem 'minitest-utils'
Expand All @@ -22,7 +24,7 @@ platforms :mri do
gem 'ruby-prof'
end

platforms :jruby do
gem 'jruby-openssl'
gem 'activerecord-jdbcsqlite3-adapter'
end
#platforms :jruby do
#gem 'jruby-openssl'
#gem 'activerecord-jdbcsqlite3-adapter'
#end
3 changes: 2 additions & 1 deletion lib/sidekiq.rb
Expand Up @@ -29,7 +29,8 @@ module Sidekiq
shutdown: [],
},
dead_max_jobs: 10_000,
dead_timeout_in_seconds: 180 * 24 * 60 * 60 # 6 months
dead_timeout_in_seconds: 180 * 24 * 60 * 60, # 6 months
reloader: proc { |&block| block.call },
}

DEFAULT_WORKER_OPTIONS = {
Expand Down
4 changes: 4 additions & 0 deletions lib/sidekiq/cli.rb
Expand Up @@ -231,6 +231,10 @@ def boot_system
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change obviates the above forced eager_load, along with the contortions it requires (for Rails 5.0+, of course).

(Or at least it would, if you still wrapped with the @app.executor when cache_classes is true.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't have the context for your comment. Could you be more specific?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The block of code just above here (231-235) exists to force eager loading, which (I assume) is because runtime autoloading and threaded workers didn't get along so well.

Autoloading is now threadsafe, as long as the relevant code is wrapped by @app.executor. @app.reloader does that internally, but the condition below is skipping the reloader when cache_classes is true.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, updated, see the commit I named you in.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

require 'sidekiq/rails'
require File.expand_path("#{options[:require]}/config/environment.rb")

if ::Rails::VERSION::MAJOR > 4 && ::Rails.env.development?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this check if Rails.configuration.cache_classes is set to false instead of relying on a specific environment name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback!

On Wed, Jun 15, 2016 at 4:15 PM, Edward Rudd notifications@github.com
wrote:

In lib/sidekiq/cli.rb
#2457 (comment):

@@ -231,6 +231,10 @@ def boot_system
end
require 'sidekiq/rails'
require File.expand_path("#{options[:require]}/config/environment.rb")
+

  •      if ::Rails::VERSION::MAJOR > 4 && ::Rails.env.development?
    

Shouldn't this check if Rails.configuration.cache_classes is set to false
instead of relying on a specific environment name?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/mperham/sidekiq/pull/2457/files/e4d09527c43816706587d86eef3b4036128b465e#r67263057,
or mute the thread
https://github.com/notifications/unsubscribe/AAALXwBfTjj6BiHKfUxGjfJwQiElR9Zeks5qMIedgaJpZM4Ffbrt
.

Sidekiq.options[:reloader] = Sidekiq::Rails::Reloader.new
end
end
options[:tag] ||= default_tag
else
Expand Down
53 changes: 28 additions & 25 deletions lib/sidekiq/processor.rb
Expand Up @@ -35,6 +35,7 @@ def initialize(mgr)
@job = nil
@thread = nil
@strategy = (mgr.options[:fetch] || Sidekiq::BasicFetch).new(mgr.options)
@reloader = Sidekiq.options[:reloader]
end

def terminate(wait=false)
Expand Down Expand Up @@ -117,33 +118,35 @@ def process(work)
jobstr = work.job
queue = work.queue_name

ack = false
begin
job = Sidekiq.load_json(jobstr)
klass = job['class'.freeze].constantize
worker = klass.new
worker.jid = job['jid'.freeze]

stats(worker, job, queue) do
Sidekiq.server_middleware.invoke(worker, job, 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['args'.freeze]))
@reloader.call do
ack = false
begin
job = Sidekiq.load_json(jobstr)
klass = job['class'.freeze].constantize
worker = klass.new
worker.jid = job['jid'.freeze]

stats(worker, job, queue) do
Sidekiq.server_middleware.invoke(worker, job, 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['args'.freeze]))
end
end
ack = true
rescue Sidekiq::Shutdown
# Had to force kill this job because it didn't finish
# within the timeout. Don't acknowledge the work since
# we didn't properly finish it.
ack = false
rescue Exception => ex
handle_exception(ex, job || { :job => jobstr })
raise
ensure
work.acknowledge if ack
end
ack = true
rescue Sidekiq::Shutdown
# Had to force kill this job because it didn't finish
# within the timeout. Don't acknowledge the work since
# we didn't properly finish it.
ack = false
rescue Exception => ex
handle_exception(ex, job || { :job => jobstr })
raise
ensure
work.acknowledge if ack
end
end

Expand Down
17 changes: 17 additions & 0 deletions lib/sidekiq/rails.rb
Expand Up @@ -34,5 +34,22 @@ class Rails < ::Rails::Engine
initializer 'sidekiq' do
Sidekiq.hook_rails!
end

class Reloader
def initialize(app = ::Rails.application)
Sidekiq.logger.debug "Enabling Rails 5+ development code reloading, so hot!"
@app = app
end

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

def inspect
"#<Sidekiq::Rails::Reloader @app=#{@app.class.name}>"
end
end
end if defined?(::Rails)
end
19 changes: 10 additions & 9 deletions myapp/Gemfile
@@ -1,21 +1,22 @@
source 'https://rubygems.org'

gem 'pry'
gem 'sidekiq', :path => '..'
gem 'rails', '5.0.0.beta4'
gem 'rack', '2.0.0.alpha'
gem 'sinatra', github: 'sinatra/sinatra'

platforms :ruby do
gem 'sqlite3'
gem 'redis-namespace'
end

platforms :jruby do
gem 'jruby-openssl'
gem 'activerecord-jdbcsqlite3-adapter'
end
#platforms :jruby do
#gem 'jruby-openssl'
#gem 'activerecord-jdbcsqlite3-adapter'
#end

gem 'rails'
gem 'sidekiq', :path => '..'
#gem 'ruby-prof'

#de Does not work with jruby or rbx:
#de gem 'pry-byebug'

# sidekiq-web dependencies
gem 'sinatra'
4 changes: 2 additions & 2 deletions sidekiq.gemspec
Expand Up @@ -19,8 +19,8 @@ Gem::Specification.new do |gem|
gem.add_dependency 'connection_pool', '~> 2.2', '>= 2.2.0'
gem.add_dependency 'concurrent-ruby', '~> 1.0'
gem.add_development_dependency 'redis-namespace', '~> 1.5', '>= 1.5.2'
gem.add_development_dependency 'sinatra', '~> 1.4', '>= 1.4.6'
gem.add_development_dependency 'sinatra', '>= 1.4.6'
gem.add_development_dependency 'minitest', '~> 5.7', '>= 5.7.0'
gem.add_development_dependency 'rake', '~> 10.0'
gem.add_development_dependency 'rails', '~> 4', '>= 3.2.0'
gem.add_development_dependency 'rails', '>= 3.2.0'
end