Permalink
Browse files

FileUpdateChecker should be able to handle deleted files.

  • Loading branch information...
1 parent 1f5b9bb commit 80256abb39332dd49996b909d6f0413a15291a90 @josevalim josevalim committed Dec 13, 2011
@@ -95,8 +95,7 @@ class Railtie < Rails::Railtie
end
initializer "active_record.add_watchable_files" do |app|
- files = ["#{app.root}/db/schema.rb", "#{app.root}/db/structure.sql"]
- config.watchable_files.concat files.select { |f| File.exist?(f) }
+ config.watchable_files.concat ["#{app.root}/db/schema.rb", "#{app.root}/db/structure.sql"]
end
config.after_initialize do
@@ -2,10 +2,27 @@
require "active_support/core_ext/array/extract_options"
module ActiveSupport
- # This class is responsible to track files and invoke the given block
- # whenever one of these files are changed. For example, this class
- # is used by Rails to reload the I18n framework whenever they are
- # changed upon a new request.
+ # \FileUpdateChecker specifies the API used by Rails to watch files
+ # and control reloading. The API depends on four methods:
+ #
+ # * +initialize+ which expects two parameters and one block as
+ # described below;
+ #
+ # * +updated?+ which returns a boolean if there were updates in
+ # the filesystem or not;
+ #
+ # * +execute+ which executes the given block on initialization
+ # and updates the counter to the latest timestamp;
+ #
+ # * +execute_if_updated+ which just executes the block if it was updated;
+ #
+ # After initialization, a call to +execute_if_updated+ must execute
+ # the block only if there was really a change in the filesystem.
+ #
+ # == Examples
+ #
+ # This class is used by Rails to reload the I18n framework whenever
+ # they are changed upon a new request.
#
# i18n_reloader = ActiveSupport::FileUpdateChecker.new(paths) do
# I18n.reload!
@@ -16,54 +33,54 @@ module ActiveSupport
# end
#
class FileUpdateChecker
- # It accepts two parameters on initialization. The first is
- # the *paths* and the second is *calculate*, a boolean.
- #
- # paths must be an array of file paths but can contain a hash as
- # last argument. The hash must have directories as keys and the
- # value is an array of extensions to be watched under that directory.
+ # It accepts two parameters on initialization. The first is an array
+ # of files and the second is an optional hash of directories. The hash must
+ # have directories as keys and the value is an array of extensions to be
+ # watched under that directory.
#
- # If *calculate* is true, the latest updated at will calculated
- # on initialization, therefore, the first call to execute_if_updated
- # will only evaluate the block if something really changed.
+ # This method must also receive a block that will be called once a path changes.
#
- # This method must also receive a block that will be called once a file changes.
+ # == Implementation details
#
- # This particular implementation checks for added files and updated files,
+ # This particular implementation checks for added and updated files,
# but not removed files. Directories lookup are compiled to a glob for
- # performance. Therefore, while someone can add new files to paths after
- # initialization, adding new directories is not allowed. Notice that,
- # depending on the implementation, not even new files may be added.
- def initialize(paths, calculate=false, &block)
- @paths = paths
- @glob = compile_glob(@paths.extract_options!)
+ # performance. Therefore, while someone can add new files to the +files+
+ # array after initialization (and parts of Rails do depend on this feature),
+ # adding new directories after initialization is not allowed.
+ #
+ # Notice that other objects that implements FileUpdateChecker API may
+ # not even allow new files to be added after initialization. If this
+ # is the case, we recommend freezing the +files+ after initialization to
+ # avoid changes that won't make effect.
+ def initialize(files, dirs={}, &block)
@jeffrafter
jeffrafter Jan 17, 2012

This change is not backwards compatible for the defaults used by most engines (e.g. forem, others). Can we at least get a deprecation notice? I can work up a sample if we need a failing test.

@josevalim
josevalim Jan 17, 2012 Ruby on Rails member

Sorry, but I don't understand exactly what do you mean. This class is pretty much internal to Rails. The documentation is only for people that wishes to implement their own file updater. In other words, this implementation is internal to Rails, but the contract used by the reloader should be stable from 3.2 on. Maybe we should add such a note to this class docs?

@jeffrafter
jeffrafter via email Jan 17, 2012
@josevalim
josevalim Jan 17, 2012 Ruby on Rails member

Which API from the file update checker are you using in your engines? What do you want to achieve? We should have a public API for whatever you want to do (via Rails.application or Rails::Engine) instead of changing the internals. FileUpdateChecker is an implementation detail.

+ @files = files
+ @glob = compile_glob(dirs)
@block = block
@updated_at = nil
- @last_update_at = calculate ? updated_at : nil
+ @last_update_at = updated_at
end
# Check if any of the entries were updated. If so, the updated_at
# value is cached until the block is executed via +execute+ or +execute_if_updated+
def updated?
current_updated_at = updated_at
- if @last_update_at != current_updated_at
+ if @last_update_at < current_updated_at
@updated_at = updated_at
true
else
false
end
end
- # Executes the given block expiring any internal cache.
+ # Executes the given block and updates the counter to latest timestamp.
def execute
@last_update_at = updated_at
@block.call
ensure
@updated_at = nil
end
- # Execute the block given if updated. This call
- # always flush the cache.
+ # Execute the block given if updated.
def execute_if_updated
if updated?
execute
@@ -78,9 +95,9 @@ def execute_if_updated
def updated_at #:nodoc:
@updated_at || begin
all = []
- all.concat @paths
+ all.concat @files.select { |f| File.exists?(f) }
all.concat Dir[@glob] if @glob
- all.map { |path| File.mtime(path) }.max
+ all.map { |path| File.mtime(path) }.max || Time.at(0)
end
end
@@ -64,7 +64,7 @@ def self.initialize_i18n(app)
init_fallbacks(fallbacks) if fallbacks && validate_fallbacks(fallbacks)
reloader_paths.concat I18n.load_path
- reloader.execute_if_updated
+ reloader.execute
@i18n_inited = true
end
@@ -14,7 +14,7 @@ def setup
def teardown
FileUtils.rm_rf("tmp_watcher")
- FileUtils.rm(FILES)
+ FileUtils.rm_rf(FILES)
end
def test_should_not_execute_the_block_if_no_paths_are_given
@@ -24,39 +24,33 @@ def test_should_not_execute_the_block_if_no_paths_are_given
assert_equal 0, i
end
- def test_should_invoke_the_block_on_first_call_if_it_does_not_calculate_last_updated_at_on_load
- i = 0
- checker = ActiveSupport::FileUpdateChecker.new(FILES){ i += 1 }
- checker.execute_if_updated
- assert_equal 1, i
- end
-
- def test_should_not_invoke_the_block_on_first_call_if_it_calculates_last_updated_at_on_load
- i = 0
- checker = ActiveSupport::FileUpdateChecker.new(FILES, true){ i += 1 }
- checker.execute_if_updated
- assert_equal 0, i
- end
-
def test_should_not_invoke_the_block_if_no_file_has_changed
i = 0
- checker = ActiveSupport::FileUpdateChecker.new(FILES, true){ i += 1 }
+ checker = ActiveSupport::FileUpdateChecker.new(FILES){ i += 1 }
5.times { assert !checker.execute_if_updated }
assert_equal 0, i
end
def test_should_invoke_the_block_if_a_file_has_changed
i = 0
- checker = ActiveSupport::FileUpdateChecker.new(FILES, true){ i += 1 }
+ checker = ActiveSupport::FileUpdateChecker.new(FILES){ i += 1 }
sleep(1)
FileUtils.touch(FILES)
assert checker.execute_if_updated
assert_equal 1, i
end
- def test_should_cache_updated_result_until_flushed
+ def test_should_be_robust_enough_to_handle_deleted_files
i = 0
- checker = ActiveSupport::FileUpdateChecker.new(FILES, true){ i += 1 }
+ checker = ActiveSupport::FileUpdateChecker.new(FILES){ i += 1 }
+ FileUtils.rm(FILES)
+ assert !checker.execute_if_updated
+ assert_equal 0, i
+ end
+
+ def test_should_cache_updated_result_until_execute
+ i = 0
+ checker = ActiveSupport::FileUpdateChecker.new(FILES){ i += 1 }
assert !checker.updated?
sleep(1)
@@ -69,7 +63,7 @@ def test_should_cache_updated_result_until_flushed
def test_should_invoke_the_block_if_a_watched_dir_changed_its_glob
i = 0
- checker = ActiveSupport::FileUpdateChecker.new([{"tmp_watcher" => [:txt]}], true){ i += 1 }
+ checker = ActiveSupport::FileUpdateChecker.new([], "tmp_watcher" => [:txt]){ i += 1 }
FileUtils.cd "tmp_watcher" do
FileUtils.touch(FILES)
end
@@ -79,7 +73,7 @@ def test_should_invoke_the_block_if_a_watched_dir_changed_its_glob
def test_should_not_invoke_the_block_if_a_watched_dir_changed_its_glob
i = 0
- checker = ActiveSupport::FileUpdateChecker.new([{"tmp_watcher" => :rb}], true){ i += 1 }
+ checker = ActiveSupport::FileUpdateChecker.new([], "tmp_watcher" => :rb){ i += 1 }
FileUtils.cd "tmp_watcher" do
FileUtils.touch(FILES)
end
@@ -124,7 +124,7 @@ def watchable_args
dirs[path.to_s] = [:rb]
end
- files << dirs
+ [files, dirs]
end
# Initialize the application passing the given group. By default, the
@@ -64,10 +64,9 @@ module Finisher
# routes added in the hook are still loaded.
initializer :set_routes_reloader_hook do
reloader = routes_reloader
- hook = lambda { reloader.execute_if_updated }
- hook.call
+ reloader.execute_if_updated
self.reloaders << reloader
- ActionDispatch::Reloader.to_prepare(&hook)
+ ActionDispatch::Reloader.to_prepare { reloader.execute_if_updated }
end
# Set app reload just after the finisher hook to ensure
@@ -79,7 +78,7 @@ module Finisher
end
if config.reload_classes_only_on_change
- reloader = config.file_watcher.new(watchable_args, true, &callback)
+ 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.
@@ -4,11 +4,10 @@ module Rails
class Application
class RoutesReloader
attr_reader :route_sets, :paths
- delegate :execute_if_updated, :updated?, :to => :@updater
+ delegate :execute_if_updated, :execute, :updated?, :to => :updater
- def initialize(updater=ActiveSupport::FileUpdateChecker)
+ def initialize
@paths = []
- @updater = updater.new(paths) { reload! }
@route_sets = []
end
@@ -20,7 +19,15 @@ def reload!
revert
end
- protected
+ private
+
+ def updater
+ @updater ||= begin
+ updater = ActiveSupport::FileUpdateChecker.new(paths) { reload! }
+ updater.execute
+ updater
+ end
+ end
def clear!
route_sets.each do |routes|
@@ -175,6 +175,38 @@ def self.counter; 2; end
assert_equal "1", last_response.body
end
+ test "added files also trigger reloading" do
+ add_to_config <<-RUBY
+ config.cache_classes = false
+ RUBY
+
+ app_file 'config/routes.rb', <<-RUBY
+ $counter = 0
+ AppTemplate::Application.routes.draw do
+ match '/c', :to => lambda { |env| User; [200, {"Content-Type" => "text/plain"}, [$counter.to_s]] }
+ end
+ RUBY
+
+ app_file "app/models/user.rb", <<-MODEL
+ class User
+ $counter += 1
+ end
+ MODEL
+
+ require 'rack/test'
+ extend Rack::Test::Methods
+
+ require "#{rails_root}/config/environment"
+
+ get "/c"
+ assert_equal "1", last_response.body
+
+ app_file "db/schema.rb", ""
+
+ get "/c"
+ assert_equal "2", last_response.body
+ end
+
protected
def setup_ar!

0 comments on commit 80256ab

Please sign in to comment.