Merge pull request rails#47347 from jhawthorn/view_cache_main_reloader

Use "main" reloader and cache resolvers from {append,prepend}_view_path

Author: jhawthorn

actionpack/lib/action_dispatch/middleware/exception_wrapper.rb

@@ -245,11 +245,9 @@ def spot(exc)
       def build_backtrace
         built_methods = {}
 
-        ActionView::ViewPaths.all_view_paths.each do |path_set|
-          path_set.each do |resolver|
-            resolver.built_templates.each do |template|
-              built_methods[template.method_name] = template
-            end
+        ActionView::ViewPaths::Registry.all_resolvers.each do |resolver|
+          resolver.built_templates.each do |template|
+            built_methods[template.method_name] = template
           end
         end
 

actionview/lib/action_view/cache_expiry.rb

@@ -1,65 +1,65 @@
 # frozen_string_literal: true
 
 module ActionView
-  class CacheExpiry
-    class Executor
-      def initialize(watcher:)
-        @execution_lock = Concurrent::ReentrantReadWriteLock.new
-        @cache_expiry = ViewModificationWatcher.new(watcher: watcher) do
-          clear_cache
+  module CacheExpiry # :nodoc: all
+    class ViewReloader
+      def initialize(watcher:, &block)
+        @mutex = Mutex.new
+        @watcher_class = watcher
+        @watched_dirs = nil
+        @watcher = nil
+        @previous_change = false
+
+        rebuild_watcher
+
+        _self = self
+        ActionView::ViewPaths::Registry.singleton_class.set_callback(:build_file_system_resolver, :after) do
+          _self.send(:rebuild_watcher)
         end
       end
 
-      def run
-        ActiveSupport::Dependencies.interlock.permit_concurrent_loads do
-          @cache_expiry.execute_if_updated
-          @execution_lock.acquire_read_lock
-        end
+      def updated?
+        @previous_change || @watcher.updated?
       end
 
-      def complete(_)
-        @execution_lock.release_read_lock
+      def execute
+        watcher = nil
+        @mutex.synchronize do
+          @previous_change = false
+          watcher = @watcher
+        end
+        watcher.execute
       end
 
       private
-        def clear_cache
-          @execution_lock.with_write_lock do
-            ActionView::LookupContext::DetailsKey.clear
-          end
+        def reload!
+          ActionView::LookupContext::DetailsKey.clear
         end
-    end
 
-    class ViewModificationWatcher
-      def initialize(watcher:, &block)
-        @watched_dirs = nil
-        @watcher_class = watcher
-        @watcher = nil
-        @mutex = Mutex.new
-        @block = block
-      end
+        def rebuild_watcher
+          @mutex.synchronize do
+            old_watcher = @watcher
 
-      def execute_if_updated
-        @mutex.synchronize do
-          watched_dirs = dirs_to_watch
-          return if watched_dirs.empty?
+            if @watched_dirs != dirs_to_watch
+              @watched_dirs = dirs_to_watch
+              new_watcher = @watcher_class.new([], @watched_dirs) do
+                reload!
+              end
+              @watcher = new_watcher
 
-          if watched_dirs != @watched_dirs
-            @watched_dirs = watched_dirs
-            @watcher = @watcher_class.new([], watched_dirs, &@block)
-            @watcher.execute
-          else
-            @watcher.execute_if_updated
+              # We must check the old watcher after initializing the new one to
+              # ensure we don't miss any events
+              @previous_change ||= old_watcher&.updated?
+            end
           end
         end
-      end
 
-      private
         def dirs_to_watch
-          all_view_paths.grep(FileSystemResolver).map!(&:path).tap(&:uniq!).sort!
+          all_view_paths.uniq.sort
         end
 
         def all_view_paths
-          ActionView::ViewPaths.all_view_paths.flat_map(&:paths)
+          ActionView::ViewPaths::Registry.all_file_system_resolvers.map(&:path)
         end
     end
   end

actionview/lib/action_view/lookup_context.rb

@@ -71,8 +71,8 @@ def self.details_cache_key(details)
       end
 
       def self.clear
-        ActionView::ViewPaths.all_view_paths.each do |path_set|
-          path_set.each(&:clear_cache)
+        ActionView::ViewPaths::Registry.all_resolvers.each do |resolver|
+          resolver.clear_cache
         end
         @view_context_class = nil
         @details_keys.clear

actionview/lib/action_view/path_set.rb

@@ -67,7 +67,7 @@ def typecast(paths)
         paths.map do |path|
           case path
           when Pathname, String
-            FileSystemResolver.new path.to_s
+            ViewPaths::Registry.file_system_resolver(path.to_s)
           when Resolver
             path
           else

actionview/lib/action_view/railtie.rb

@@ -107,7 +107,14 @@ class Railtie < Rails::Engine # :nodoc:
       end
 
       unless enable_caching
-        app.executor.register_hook ActionView::CacheExpiry::Executor.new(watcher: app.config.file_watcher)
+        view_reloader = ActionView::CacheExpiry::ViewReloader.new(watcher: app.config.file_watcher)
+
+        app.reloaders << view_reloader
+        view_reloader.execute
+        app.reloader.to_run do
+          require_unload_lock!
+          view_reloader.execute
+        end
       end
     end
 

actionview/lib/action_view/view_paths.rb

@@ -5,19 +5,19 @@ module ViewPaths
     extend ActiveSupport::Concern
 
     included do
-      ViewPaths.set_view_paths(self, ActionView::PathSet.new.freeze)
+      ViewPaths::Registry.set_view_paths(self, ActionView::PathSet.new.freeze)
     end
 
     delegate :template_exists?, :any_templates?, :view_paths, :formats, :formats=,
              :locale, :locale=, to: :lookup_context
 
     module ClassMethods
       def _view_paths
-        ViewPaths.get_view_paths(self)
+        ViewPaths::Registry.get_view_paths(self)
       end
 
       def _view_paths=(paths)
-        ViewPaths.set_view_paths(self, paths)
+        ViewPaths::Registry.set_view_paths(self, paths)
       end
 
       def _prefixes # :nodoc:
@@ -70,21 +70,46 @@ def local_prefixes
         end
     end
 
-    # :stopdoc:
-    @all_view_paths = {}
+    module Registry # :nodoc:
+      @view_paths_by_class = {}
+      @file_system_resolvers = Concurrent::Map.new
 
-    def self.get_view_paths(klass)
-      @all_view_paths[klass] || get_view_paths(klass.superclass)
-    end
+      class << self
+        include ActiveSupport::Callbacks
+        define_callbacks :build_file_system_resolver
+      end
 
-    def self.set_view_paths(klass, paths)
-      @all_view_paths[klass] = paths
-    end
+      def self.get_view_paths(klass)
+        @view_paths_by_class[klass] || get_view_paths(klass.superclass)
+      end
+
+      def self.set_view_paths(klass, paths)
+        @view_paths_by_class[klass] = paths
+      end
 
-    def self.all_view_paths
-      @all_view_paths.values.uniq
+      def self.file_system_resolver(path)
+        path = File.expand_path(path)
+        resolver = @file_system_resolvers[path]
+        unless resolver
+          run_callbacks(:build_file_system_resolver) do
+            resolver = @file_system_resolvers.fetch_or_store(path) do
+              FileSystemResolver.new(path)
+            end
+          end
+        end
+        resolver
+      end
+
+      def self.all_resolvers
+        resolvers = [all_file_system_resolvers]
+        resolvers.concat @view_paths_by_class.values.map(&:to_a)
+        resolvers.flatten.uniq
+      end
+
+      def self.all_file_system_resolvers
+        @file_system_resolvers.values
+      end
     end
-    # :startdoc:
 
     # The prefixes used in render "foo" shortcuts.
     def _prefixes # :nodoc:

actionview/test/actionpack/controller/render_test.rb

@@ -1471,35 +1471,40 @@ def test_render_call_to_partial_with_layout_in_main_layout_and_within_content_fo
     assert_equal "Before (Anthony)\nInside from partial (Anthony)\nAfter\nBefore (David)\nInside from partial (David)\nAfter\nBefore (Ramm)\nInside from partial (Ramm)\nAfter", @response.body
   end
 
-  def test_template_annotations
+  def with_annotations_enabled
     ActionView::Base.annotate_rendered_view_with_filenames = true
+    ActionView::LookupContext::DetailsKey.clear
+    yield
+  ensure
+    ActionView::Base.annotate_rendered_view_with_filenames = false
+    ActionView::LookupContext::DetailsKey.clear
+  end
 
-    get :greeting
+  def test_template_annotations
+    with_annotations_enabled do
+      get :greeting
+    end
 
     assert_includes @response.body, "<!-- BEGIN"
     assert_includes @response.body, "<!-- END"
     assert_includes @response.body, "test/fixtures/actionpack/test/greeting.html.erb"
     assert_includes @response.body, "This is grand!"
-  ensure
-    ActionView::Base.annotate_rendered_view_with_filenames = false
   end
 
   def test_template_annotations_do_not_render_for_non_html_format
-    ActionView::Base.annotate_rendered_view_with_filenames = true
-
-    get :render_with_explicit_template_with_locals
+    with_annotations_enabled do
+      get :render_with_explicit_template_with_locals
+    end
 
     assert_not_includes @response.body, "BEGIN"
     assert_equal @response.body.split("\n").length, 1
-  ensure
-    ActionView::Base.annotate_rendered_view_with_filenames = false
   end
 
   def test_line_offset_with_annotations_enabled
-    ActionView::Base.annotate_rendered_view_with_filenames = true
-
     exc = assert_raises ActionView::Template::Error do
-      get :render_line_offset
+      with_annotations_enabled do
+        get :render_line_offset
+      end
     end
     line = exc.backtrace.first
     assert(line =~ %r{:(\d+):})

actionview/test/actionpack/controller/view_paths_test.rb

@@ -102,6 +102,28 @@ def test_template_prepends_view_path_correctly
     assert_paths TestController, *class_view_paths
   end
 
+  def test_append_view_path_does_not_bust_template_cache
+    template_instances = 2.times.map do
+      controller = Test::SubController.new
+      controller.append_view_path "#{FIXTURE_LOAD_PATH}/override2"
+      controller.lookup_context.find_all("layouts/test/sub")
+    end
+
+    object_ids = template_instances.flatten.map(&:object_id)
+    assert_equal 1, object_ids.uniq.count
+  end
+
+  def test_prepend_view_path_does_not_bust_template_cache
+    template_instances = 2.times.map do
+      controller = TestController.new
+      controller.prepend_view_path "#{FIXTURE_LOAD_PATH}/override"
+      controller.lookup_context.find_all("hello_world", "test")
+    end
+
+    object_ids = template_instances.flatten.map(&:object_id)
+    assert_equal 1, object_ids.uniq.count
+  end
+
   def test_view_paths
     get :hello_world
     assert_response :success

railties/test/application/per_request_digest_cache_test.rb

@@ -62,7 +62,7 @@ def index
   end
 
   test "template digests are cleared before a request" do
-    assert_called(ActionView::LookupContext::DetailsKey, :clear, times: 3) do
+    assert_called(ActionView::LookupContext::DetailsKey, :clear, times: 2) do
       get "/customers"
       assert_equal 200, last_response.status
     end

railties/test/application/view_reloading_test.rb

@@ -0,0 +1,56 @@
+# frozen_string_literal: true
+
+require "isolation/abstract_unit"
+require "rack/test"
+
+module ApplicationTests
+  class ViewReloadingTest < ActiveSupport::TestCase
+    include ActiveSupport::Testing::Isolation
+    include Rack::Test::Methods
+
+    def setup
+      build_app
+
+      app_file "config/routes.rb", <<-RUBY
+        Rails.application.routes.draw do
+          get 'pages/:id', to: 'pages#show'
+        end
+      RUBY
+
+      app_file "app/controllers/pages_controller.rb", <<-RUBY
+        class PagesController < ApplicationController
+          layout false
+
+          def show
+          end
+        end
+      RUBY
+    end
+
+    def teardown
+      teardown_app
+    end
+
+    test "views are reloaded" do
+      app_file "app/views/pages/show.html.erb", <<-RUBY
+        Before!
+      RUBY
+
+      ENV["RAILS_ENV"] = "development"
+      require "#{app_path}/config/environment"
+
+      get "/pages/foo"
+      get "/pages/foo"
+      assert_equal 200, last_response.status, last_response.body
+      assert_equal "Before!", last_response.body.strip
+
+      app_file "app/views/pages/show.html.erb", <<-RUBY
+        After!
+      RUBY
+
+      get "/pages/foo"
+      assert_equal 200, last_response.status
+      assert_equal "After!", last_response.body.strip
+    end
+  end
+end