From 80256abb39332dd49996b909d6f0413a15291a90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 13 Dec 2011 11:23:21 +0100 Subject: [PATCH] FileUpdateChecker should be able to handle deleted files. --- activerecord/lib/active_record/railtie.rb | 3 +- .../lib/active_support/file_update_checker.rb | 73 ++++++++++++------- .../lib/active_support/i18n_railtie.rb | 2 +- .../test/file_update_checker_test.rb | 36 ++++----- railties/lib/rails/application.rb | 2 +- railties/lib/rails/application/finisher.rb | 7 +- .../lib/rails/application/routes_reloader.rb | 15 +++- railties/test/application/loading_test.rb | 32 ++++++++ 8 files changed, 109 insertions(+), 61 deletions(-) diff --git a/activerecord/lib/active_record/railtie.rb b/activerecord/lib/active_record/railtie.rb index 71772529d2..b28e8da24c 100644 --- a/activerecord/lib/active_record/railtie.rb +++ b/activerecord/lib/active_record/railtie.rb @@ -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 diff --git a/activesupport/lib/active_support/file_update_checker.rb b/activesupport/lib/active_support/file_update_checker.rb index 3028fbdd00..a4ad2da137 100644 --- a/activesupport/lib/active_support/file_update_checker.rb +++ b/activesupport/lib/active_support/file_update_checker.rb @@ -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,37 +33,38 @@ 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) + @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 @@ -54,7 +72,7 @@ def updated? 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 @@ -62,8 +80,7 @@ def execute @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 diff --git a/activesupport/lib/active_support/i18n_railtie.rb b/activesupport/lib/active_support/i18n_railtie.rb index ae3a39562c..bbeb8d82c6 100644 --- a/activesupport/lib/active_support/i18n_railtie.rb +++ b/activesupport/lib/active_support/i18n_railtie.rb @@ -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 diff --git a/activesupport/test/file_update_checker_test.rb b/activesupport/test/file_update_checker_test.rb index c54ba427fa..dd19b58aa2 100644 --- a/activesupport/test/file_update_checker_test.rb +++ b/activesupport/test/file_update_checker_test.rb @@ -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 diff --git a/railties/lib/rails/application.rb b/railties/lib/rails/application.rb index 75f2b9a3bd..22689cc278 100644 --- a/railties/lib/rails/application.rb +++ b/railties/lib/rails/application.rb @@ -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 diff --git a/railties/lib/rails/application/finisher.rb b/railties/lib/rails/application/finisher.rb index 064723c1e0..2ce2980b97 100644 --- a/railties/lib/rails/application/finisher.rb +++ b/railties/lib/rails/application/finisher.rb @@ -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. diff --git a/railties/lib/rails/application/routes_reloader.rb b/railties/lib/rails/application/routes_reloader.rb index e080481976..ef7e733ce4 100644 --- a/railties/lib/rails/application/routes_reloader.rb +++ b/railties/lib/rails/application/routes_reloader.rb @@ -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| diff --git a/railties/test/application/loading_test.rb b/railties/test/application/loading_test.rb index 5fb04cb3b3..9c77f6210a 100644 --- a/railties/test/application/loading_test.rb +++ b/railties/test/application/loading_test.rb @@ -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! -- GitLab