FileUpdateChecker should be able to handle deleted files.

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 @@ module ActiveRecord
     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/wrap"
 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 @@ module ActiveSupport
       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 @@ module ActiveSupport
       @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 @@ module ActiveSupport
     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 @@ module I18n
       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 @@ class FileUpdateCheckerWithEnumerableTest < Test::Unit::TestCase
 
   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 @@ class FileUpdateCheckerWithEnumerableTest < Test::Unit::TestCase
     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 @@ class FileUpdateCheckerWithEnumerableTest < Test::Unit::TestCase
 
   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 @@ class FileUpdateCheckerWithEnumerableTest < Test::Unit::TestCase
 
   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 @@ module Rails
         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 Rails
       # 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 Rails
         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 @@ module Rails
         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 @@ class LoadingTest < Test::Unit::TestCase
     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!