Enable code reloading in development mode with Rails 5 #2457

Merged
merged 14 commits into from Aug 24, 2016

Projects

None yet
@mperham
Owner
mperham commented Jul 24, 2015 edited

Until now Sidekiq has been required to eager load all application code at startup, since Rails reloading has never been thread-safe. This has changed in Rails 5! If the user is running Rails 5 in development, have Sidekiq leverage the new Interlock to safely reload app code.

Testing

gem 'rails', '5.0.0.rc1'
gem 'rack', '2.0.0.rc1'
gem 'sinatra', github: 'sinatra/sinatra'
gem 'sidekiq', github: 'mperham/sidekiq', branch: 'rails5'

Now fire up Sidekiq, edit code and create jobs. You should see code changes immediately reflected in job output.

@mperham mperham commented on an outdated diff Jul 24, 2015
sidekiq.gemspec
@@ -23,5 +23,5 @@ Gem::Specification.new do |gem|
gem.add_development_dependency 'sinatra', '~> 1.4', '>= 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', '~> 5', '>= 3.2.0'
@davydovanton davydovanton and 1 other commented on an outdated diff Jul 25, 2015
@@ -1,7 +1,8 @@
source 'https://rubygems.org'
gemspec
-gem 'rails', '~> 4.2'
+gem 'rails', path: '../rails'
@davydovanton
davydovanton Jul 25, 2015 Collaborator

Mike can you change path to github for running tests?

@mperham
mperham Jul 26, 2015 Owner

You're a committer. :-) Do it.

On Jul 25, 2015, at 02:20, Anton Davydov notifications@github.com wrote:

In Gemfile:

@@ -1,7 +1,8 @@
source 'https://rubygems.org'
gemspec

-gem 'rails', '~> 4.2'
+gem 'rails', path: '../rails'
Mike can you change path to github for running tests?


Reply to this email directly or view it on GitHub.

@davydovanton
Collaborator

Great work 👍
What about status of this PR?

@mperham
Owner
mperham commented Jul 26, 2015

Needs testing by people with real rails apps. Not sure if I should merge it or leave it in the branch.

On Jul 25, 2015, at 02:32, Anton Davydov notifications@github.com wrote:

Great work
What about status of this PR?


Reply to this email directly or view it on GitHub.

@mperham
Owner
mperham commented Aug 31, 2015

Currently blocked by rails/rails#21006

@mperham Live code reloading, so hot right now!
51347c5
@mperham
Owner
mperham commented Feb 2, 2016

Appears to work with Rails 5.0.0.beta2!

@mperham modern rubies only
e9eb6a3
@seuros
Collaborator
seuros commented on e9eb6a3 Feb 2, 2016

cool

@csxiaodiao

powerful

@connorshea

Any updates on this? Will it be merged once Rails 5 is out?

@mperham
Owner
mperham commented Apr 13, 2016

Correct, until Rails 5 is released, I won't merge/support the feature.

@mperham Merge branch 'master' into rails5
eca6424
@houmanka
houmanka commented May 5, 2016 edited

Hi guys, not sure if I am allowed to post this in here.
When I try to run the sidekiq after updating it I get the following error:
uninitialized constant ActionController::RackDelegation

@mattbrictson mattbrictson added a commit to mattbrictson/rails-template that referenced this pull request May 16, 2016
@mattbrictson mattbrictson Remove Sidekiq web (doesn't work with Rails 5 yet)
Requires newer versions of Sidekiq and Sinatra, both of which haven't been
released yet.

See: mperham/sidekiq#2457
4adb55f
@urkle urkle and 1 other commented on an outdated diff Jun 15, 2016
lib/sidekiq/cli.rb
@@ -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?
@urkle
urkle Jun 15, 2016

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

@mperham
mperham Jun 16, 2016 Owner

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
.

added some commits Jun 15, 2016
@mperham merge master a67c9b1
@mperham Use cache_classes, rather than environment, to enable reloading
fbb074f
@mperham merge master
19f57d9
@mperham merge master
d8bd483
@mperham notes
5fd622e
@matthewd matthewd commented on the diff Jul 1, 2016
lib/sidekiq/cli.rb
@@ -235,6 +235,10 @@ def boot_system
end
@matthewd
matthewd Jul 1, 2016

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.)

@mperham
mperham Jul 1, 2016 Owner

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

@matthewd
matthewd Jul 1, 2016

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.

@mperham
mperham Jul 1, 2016 Owner

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

@mperham Refactor Rails 5 integration, per @matthewd
871626c
@Xosmond
Xosmond commented Jul 4, 2016

So Rails 5 is released

@fusion2004

And we will be happy when the feature is merged, whenever that may be @mperham. Thanks for all your hard work!

@mperham
Owner
mperham commented Jul 5, 2016

Sinatra hasn't released a tag or gem which supports Rack 2.0. I'm reluctant to merge changes which depend on a dependency's master. If you want to run this code, it's easy enough to pull in the branch.

added some commits Aug 20, 2016
@mperham merge master
86e03e9
@mperham merge master
0f166de
@mperham cleanup
1ab8883
@mperham mperham merged commit be3073a into master Aug 24, 2016

1 of 2 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mperham mperham deleted the rails5 branch Aug 24, 2016
@jrochkind

@matthewd / @thedarkone : Any chance of any documentation or brief overview or pointers to relevant classes for "the new Interlock API"? I'm having trouble understanding what's going on enough to use in my own non-sidekick code that does concurrency such that I've never been able to use dev-mode class-reloading before.

This code looks like it mainly uses Rails.application.reloader.wrap, and one might guess that Rails.application.reloader is an ActionDispatch::Reloader, but the generated docs for that class don't even include the existence of a wrap method, let alone docs for what it does and how it is to be used. The Rails Guide on Autoloading and Reloading Constants also does not seem to have been updated for relevant changes in Rails5.

Any hints available anywhere?

@matthewd

@jrochkind it's an ActiveSupport::Reloader. The primary PRs are rails/rails#17102 (the interlock API), and rails/rails#23807 (the executor/reloader abstraction).

The relevant API documentation is very vague, partly because that's the point of the abstraction: the API is not offering to do A and B and C for you, but rather to take care of whatever details are necessary to allow your supplied block of code to run safely / with the latest available version of any reloadable code -- and it's a hook mechanism whose actual behaviour will differ depending on what other components/libraries are in play.

While that's a conscious choice for the API, I agree we also need a more detailed discussion of what it actually does, today, when the full framework is loaded -- probably in a guide.

Meanwhile, please feel free to open a "the executor/reloader API is poorly documented" issue on Rails, and then use that to ask any questions you have: that'll both get you the information you need, and help us identify what such a document needs to cover.

@jrochkind

Thanks @matthewd! With regard to it being intended to be an abstraction, yeah, followed, what's needed is documentation for how one is meant to write code using the new Interlock API such that your code can let it "take care of whatever details are necessary to allow your supplied block of code to run safely", as you say. That's exactly what I want to know.

I don't necessarily need to know what goes on under the hood in current version of Rails -- although an understanding of that is probably a good idea if I'm going to need to debug (which I probably am, either bugs in my own code or in Rails', which happen). But first priority is documentation of the abstraction as abstraction, what is the 'contract', what is it meant to do, how am I meant to use it as an author of code that requires it's protections (like sidekiq).

I'll file an Issue with these sorts of comments, if you think it will be helpful. I'll tag you in the issue if you don't mind, because my experience of filing doc issues on Rails is generally that they simply get closed with a comment to submit better docs if I have it. If I understood it enough to write docs being sure I wasn't misleading anyone, I would, but there's a chicken and egg! (I have some doc commits in the Rails history, I do write docs when I can!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment