config.reload_classes_only_on_change doesn't honor model mtimes

[tonyla]

Relating to Rails 3-2-stable

I believe the config.reload_classes_only_on_change option does not act as expected.

Code starting here:
https://github.com/rails/rails/blob/3-2-stable/railties/lib/rails/application/finisher.rb#L79

if config.reload_classes_only_on_change
  reloader = config.file_watcher.new(*watchable_args, &callback)
  self.reloaders << reloader
  # We need to set a to_prepare callback regardless of the reloader result, i.e.
  # models should be reloaded if any of the reloaders (i18n, routes) were updated.
  ActionDispatch::Reloader.to_prepare(:prepend => true){ reloader.execute }
else
  ActionDispatch::Reloader.to_cleanup(&callback)
end

Is supposed to only reload dependencies if the mtimes of files have changed. That is what the class ActiveSupport::FileUpdateChecker provides as on of the default reloaders.

But ActionDispatch::Reloader.to_prepare(:prepend => true){ reloader.execute } that line will reload all the dependencies regardless if mtimes.

def execute
https://github.com/rails/rails/blob/3-2-stable/activesupport/lib/active_support/file_update_checker.rb#L76

I believe that line should be

ActionDispatch::Reloader.to_prepare(:prepend => true){ reloader.execute_if_updated }

def execute_if_updated
https://github.com/rails/rails/blob/3-2-stable/activesupport/lib/active_support/file_update_checker.rb#L84

This way your application code is only reloaded if it has been updated.

[schneems]

@tonyla is it possible to test this functionality, maybe creating two files, touching one and making sure only the one touched is updated in the reloader? I agree with your logic, but want this functionality tested.

Does the reloader behave the same in master?

[tonyla]

@schneems

The reloader functionality is tested in this file https://github.com/rails/rails/blob/3-2-stable/activesupport/test/file_update_checker_test.rb

Does that cover what you were thinking? Or are you meaning a test that tests the reloader in context? Meaning testing of actual file reloading inside a rails environment.

Master does behave the exact same way:
https://github.com/rails/rails/blob/master/railties/lib/rails/application/finisher.rb#L87

As an anecdote this reduces my dev page loads from ~4s to < 1s. The first page load still takes 4s but each subsequent page load is < 1s. Has made a huge different in development speed in the fairly large rails app I work with.

[stouset]

This seems like a pretty important thing to fix for Rails 4 given other improvements in getting the dev environment to load quickly. What else is necessary for this to get fixed? Tests? A patch?

[tonyla]

I can create what ever is needed, I'm just unsure of how to get this actually "approved".

I've been using this on a forked version of rails and it speeds up development significantly.

https://github.com/pingg-corp/rails/tree/v3.2.12-patched

[stouset]

@tonyla is it possible to test this functionality, maybe creating two files, touching one and making sure only the one touched is updated in the reloader? I agree with your logic, but want this functionality tested.

You've obviously got a patch. I'd make a pull request referencing this issue with the relevant changes, plus tests.

[tonyla]

I did some more digging and found out that the reloading behaviour was being caused by wavii/rails-dev-tweaks#9 (A gem that I use). I'll detail the explanation below just in case it helps someone in the future.

The code does behave as expected but it is very deceiving when reading it. That is because the line

ActionDispatch::Reloader.to_prepare(:prepend => true){ reloader.execute }

Looks like it will reload on every request. But reloaders are not actually run unless #reload_dependencies? is true.

    def reload_dependencies? #:nodoc:
      config.reload_classes_only_on_change != true || reloaders.map(&:updated?).any?
    end

As you can see it checks to see if any reloader #updated?. It will not reload the code on ever request.

[rafaelfranca]

@josevalim could you take a look?