From 72abd6357d7909c15a460c716840159e5bb97ac7 Mon Sep 17 00:00:00 2001 From: John Hawthorn Date: Wed, 8 Feb 2023 20:23:06 -0800 Subject: [PATCH] Reuse "main" reloader for ActionView expiration Co-authored-by: Matthew Draper --- actionview/lib/action_view/cache_expiry.rb | 80 +++++++++---------- actionview/lib/action_view/path_set.rb | 2 +- actionview/lib/action_view/railtie.rb | 9 ++- actionview/lib/action_view/view_paths.rb | 10 +-- .../per_request_digest_cache_test.rb | 2 +- .../test/application/view_reloading_test.rb | 56 +++++++++++++ 6 files changed, 110 insertions(+), 49 deletions(-) create mode 100644 railties/test/application/view_reloading_test.rb diff --git a/actionview/lib/action_view/cache_expiry.rb b/actionview/lib/action_view/cache_expiry.rb index cab052b66bda4..c50a0ed2fa6d4 100644 --- a/actionview/lib/action_view/cache_expiry.rb +++ b/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 diff --git a/actionview/lib/action_view/path_set.rb b/actionview/lib/action_view/path_set.rb index 7cb1f25ad803a..7d2615898f3ef 100644 --- a/actionview/lib/action_view/path_set.rb +++ b/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 diff --git a/actionview/lib/action_view/railtie.rb b/actionview/lib/action_view/railtie.rb index dd3df1613780b..e2626566c1387 100644 --- a/actionview/lib/action_view/railtie.rb +++ b/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 diff --git a/actionview/lib/action_view/view_paths.rb b/actionview/lib/action_view/view_paths.rb index e3d97fad61a70..e8c9b4bb01d77 100644 --- a/actionview/lib/action_view/view_paths.rb +++ b/actionview/lib/action_view/view_paths.rb @@ -87,16 +87,14 @@ def self.set_view_paths(klass, paths) @view_paths_by_class[klass] = paths end - def self.all_resolvers - @all_view_paths.values.map(&:to_a).flatten.uniq - end - def self.file_system_resolver(path) path = File.expand_path(path) resolver = @file_system_resolvers[path] unless resolver - resolver = @file_system_resolvers.fetch_or_store(path) do - FileSystemResolver.new(path) + run_callbacks(:build_file_system_resolver) do + resolver = @file_system_resolvers.fetch_or_store(path) do + FileSystemResolver.new(path) + end end end resolver diff --git a/railties/test/application/per_request_digest_cache_test.rb b/railties/test/application/per_request_digest_cache_test.rb index 2cbddd3284cb4..809f94be8dcd1 100644 --- a/railties/test/application/per_request_digest_cache_test.rb +++ b/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 diff --git a/railties/test/application/view_reloading_test.rb b/railties/test/application/view_reloading_test.rb new file mode 100644 index 0000000000000..ac78ff89ad107 --- /dev/null +++ b/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