Enable code reloading in development mode with Rails 5 by mperham · Pull Request #2457 · sidekiq/sidekiq

mperham · 2015-07-24T20:17:26Z

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 · 2015-07-24T20:35:34Z

davydovanton · 2015-07-25T09:32:15Z

Great work 👍
What about status of this PR?

mperham · 2015-07-26T00:32:03Z

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 · 2015-08-31T15:59:08Z

Currently blocked by rails/rails#21006

mperham · 2016-02-02T00:01:14Z

Appears to work with Rails 5.0.0.beta2!

csxiaodiao · 2016-03-08T08:39:34Z

powerful

connorshea · 2016-04-13T23:42:42Z

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

mperham · 2016-04-13T23:44:27Z

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

houmanka · 2016-05-05T00:55:13Z

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

Requires newer versions of Sidekiq and Sinatra, both of which haven't been released yet. See: sidekiq/sidekiq#2457

urkle · 2016-06-15T23:15:40Z

          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?

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
.

matthewd · 2016-07-01T04:47:02Z

@@ -235,6 +235,10 @@ def boot_system
          end


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

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

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.

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

Xosmond · 2016-07-04T05:05:06Z

So Rails 5 is released

fusion2004 · 2016-07-05T15:37:32Z

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

mperham · 2016-07-05T15:47:12Z

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.

jrochkind · 2016-09-25T12:27:29Z

@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 · 2016-09-25T14:45:30Z

@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 · 2016-09-25T15:01:50Z

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

mperham mentioned this pull request Jul 24, 2015

Don't apply locking around basic #load / #require rails/rails#20989

Merged

mperham reviewed Jul 24, 2015
View reviewed changes

Comment thread sidekiq.gemspec Outdated

mperham Jul 24, 2015

Copy link
Copy Markdown

Collaborator Author

Choose a reason for hiding this comment

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

Oops.

davydovanton added the feature label Jul 29, 2015

Live code reloading, so hot right now!

51347c5

mperham force-pushed the rails5 branch from f85ef4f to 51347c5 Compare February 2, 2016 00:00

modern rubies only

e9eb6a3

Merge branch 'master' into rails5

eca6424

mperham mentioned this pull request Apr 15, 2016

Rails 5 integration with Sidekiq monitoring #2931

Closed

mperham added 2 commits April 28, 2016 10:02

Update reload API for 5.0.beta4

df002c1

polish

e4d0952

mattbrictson added a commit to mattbrictson/rails-template that referenced this pull request May 16, 2016

Remove Sidekiq web (doesn't work with Rails 5 yet)

4adb55f

Requires newer versions of Sidekiq and Sinatra, both of which haven't been released yet. See: sidekiq/sidekiq#2457

urkle reviewed Jun 15, 2016
View reviewed changes

mperham added 5 commits June 15, 2016 16:48

merge master

a67c9b1

Use cache_classes, rather than environment, to enable reloading

fbb074f

merge master

19f57d9

merge master

d8bd483

notes

5fd622e

matthewd reviewed Jul 1, 2016
View reviewed changes

Refactor Rails 5 integration, per @matthewd

871626c

mperham mentioned this pull request Aug 20, 2016

Rails' eager load hooks at not invoked #1791

Closed

mperham added 3 commits August 19, 2016 21:47

merge master

86e03e9

merge master

0f166de

cleanup

1ab8883

mperham merged commit be3073a into master Aug 24, 2016

mperham deleted the rails5 branch August 24, 2016 16:55

bdewater mentioned this pull request Oct 3, 2016

Getting a callback when processing is done jrgifford/delayed_paperclip#176

Open

This was referenced Oct 20, 2016

Rails5, concurrency in action method, deadlock, docs needed including for executor/reloader rails/rails#26847

Closed

Rails5, auto-loading, future, circular dependency ruby-concurrency/concurrent-ruby#585

Open

tahirpoduska mentioned this pull request Jan 31, 2018

Cannot define multiple 'included' blocks for a Concern, only in development environment ruby-shoryuken/shoryuken#467

Closed

Conversation

mperham commented Jul 24, 2015 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davydovanton commented Jul 25, 2015

Uh oh!

mperham commented Jul 26, 2015

Uh oh!

mperham commented Aug 31, 2015

Uh oh!

mperham commented Feb 2, 2016

Uh oh!

csxiaodiao commented Mar 8, 2016

Uh oh!

connorshea commented Apr 13, 2016

Uh oh!

mperham commented Apr 13, 2016

Uh oh!

houmanka commented May 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xosmond commented Jul 4, 2016

Uh oh!

fusion2004 commented Jul 5, 2016

Uh oh!

mperham commented Jul 5, 2016

Uh oh!

jrochkind commented Sep 25, 2016

Uh oh!

matthewd commented Sep 25, 2016

Uh oh!

jrochkind commented Sep 25, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

mperham commented Jul 24, 2015 •

edited

Loading

houmanka commented May 5, 2016 •

edited

Loading