diff --git a/actionview/lib/action_view/template.rb b/actionview/lib/action_view/template.rb index c5836bf4a20b2..9efc306a2a0da 100644 --- a/actionview/lib/action_view/template.rb +++ b/actionview/lib/action_view/template.rb @@ -408,6 +408,19 @@ def strict_locals? strict_locals! end + def freeze # :nodoc: + strict_locals! + method_name.freeze + @source.freeze + @identifier.freeze + @virtual_path&.freeze + @locals&.freeze + @strict_locals.freeze if @strict_locals.is_a?(String) + @variant.freeze if @variant.is_a?(String) + @compile_mutex = nil + super + end + # Exceptions are marshalled when using the parallel test runner with DRb, so we need # to ensure that references to the template object can be marshalled as well. This means forgoing # the marshalling of the compiler mutex and instantiating that again on unmarshalling. diff --git a/actionview/lib/action_view/template/resolver.rb b/actionview/lib/action_view/template/resolver.rb index f07f97d568fdd..4c1bc0ecdbeff 100644 --- a/actionview/lib/action_view/template/resolver.rb +++ b/actionview/lib/action_view/template/resolver.rb @@ -33,9 +33,12 @@ def build_path_regex }x end - def parse(path) + def path_regex @regex ||= build_path_regex - match = @regex.match(path) + end + + def parse(path) + match = path_regex.match(path) path = TemplatePath.build(match[:action], match[:prefix] || "", !!match[:partial]) details = TemplateDetails.new( match[:locale]&.to_sym, @@ -45,6 +48,11 @@ def parse(path) ) ParsedPath.new(path, details) end + + def freeze + path_regex + super + end end cattr_accessor :caching, default: true @@ -104,6 +112,25 @@ def clear_cache super end + def eager_load_templates + template_glob("**/*").each do |file| + unbound = build_unbound_template(file) + (@unbound_templates[unbound.virtual_path] ||= []) << unbound + end + end + + def freeze + @path.freeze + @path_parser.freeze + @unbound_templates = @unbound_templates.each_pair.to_h unless @unbound_templates.is_a?(::Hash) + @unbound_templates.each_value do |unbound_templates| + unbound_templates.each(&:freeze) + unbound_templates.freeze + end + @unbound_templates.freeze + super + end + def to_s @path.to_s end @@ -130,19 +157,27 @@ def built_templates # :nodoc: private def _find_all(name, prefix, partial, details, key, locals) requested_details = key || TemplateDetails::Requested.new(**details) - cache = key ? @unbound_templates : Concurrent::Map.new - - unbound_templates = - cache.compute_if_absent(TemplatePath.virtual(name, prefix, partial)) do - path = TemplatePath.build(name, prefix, partial) - unbound_templates_from_path(path) - end + unbound_templates = unbound_templates_for(name, prefix, partial, !key.nil?) filter_and_sort_by_details(unbound_templates, requested_details).map do |unbound_template| unbound_template.bind_locals(locals) end end + def unbound_templates_for(name, prefix, partial, cache) + virtual = TemplatePath.virtual(name, prefix, partial) + + if frozen? + @unbound_templates[virtual] || [].freeze + elsif cache + @unbound_templates.compute_if_absent(virtual) do + unbound_templates_from_path(TemplatePath.build(name, prefix, partial)) + end + else + unbound_templates_from_path(TemplatePath.build(name, prefix, partial)) + end + end + def source_for_template(template) Template::Sources::File.new(template) end diff --git a/actionview/lib/action_view/template/sources/file.rb b/actionview/lib/action_view/template/sources/file.rb index 521bd06f45f08..028f27c3fb27d 100644 --- a/actionview/lib/action_view/template/sources/file.rb +++ b/actionview/lib/action_view/template/sources/file.rb @@ -11,6 +11,11 @@ def initialize(filename) def to_s ::File.binread @filename end + + def freeze + @filename.freeze + super + end end end end diff --git a/actionview/lib/action_view/unbound_template.rb b/actionview/lib/action_view/unbound_template.rb index b9e28e2e7df71..a6fb74801ce2d 100644 --- a/actionview/lib/action_view/unbound_template.rb +++ b/actionview/lib/action_view/unbound_template.rb @@ -13,13 +13,41 @@ def initialize(source, identifier, details:, virtual_path:) @details = details @virtual_path = virtual_path + # Strict templates ignore the locals passed at render time, so a single + # template serves every key. @strict_locals_template holds that one + # template once we've discovered the template is strict; until then + # @templates caches one template per set of locals. + @strict_locals_template = nil @templates = Concurrent::Map.new(initial_capacity: 2) @write_lock = Mutex.new end def bind_locals(locals) - unless template = @templates[locals] + @strict_locals_template || @templates[locals] || build_bound_template(locals) + end + + def built_templates # :nodoc: + @strict_locals_template ? [@strict_locals_template] : @templates.values + end + + def freeze # :nodoc: + unless bind_locals([]).strict_locals? + raise ArgumentError, "Cannot freeze #{@virtual_path.inspect}: templates must declare strict locals (e.g. `<%# locals: () %>`) to be frozen." + end + @source.freeze + @identifier.freeze + @virtual_path.freeze + @details.freeze + @strict_locals_template.freeze + @templates = nil + @write_lock = nil + super + end + + private + def build_bound_template(locals) @write_lock.synchronize do + return @strict_locals_template if @strict_locals_template normalized_locals = normalize_locals(locals) # We need ||=, both to dedup on the normalized locals and to check @@ -27,25 +55,17 @@ def bind_locals(locals) template = (@templates[normalized_locals] ||= build_template(normalized_locals)) if template.strict_locals? - # Under strict locals, we only need one template. - # This replaces the @templates Concurrent::Map with a hash which - # returns this template for every key. - @templates = Hash.new(template).freeze + @strict_locals_template = template else # This may have already been assigned, but we've already de-dup'd so # reassignment is fine. @templates[locals.dup] = template end + + template end end - template - end - def built_templates # :nodoc: - @templates.values - end - - private def build_template(locals) Template.new( @source, diff --git a/actionview/test/template/file_system_resolver_test.rb b/actionview/test/template/file_system_resolver_test.rb index aa03fdcb1383d..5fe282d51b95a 100644 --- a/actionview/test/template/file_system_resolver_test.rb +++ b/actionview/test/template/file_system_resolver_test.rb @@ -9,4 +9,76 @@ class FileSystemResolverTest < ActiveSupport::TestCase def resolver ActionView::FileSystemResolver.new(tmpdir) end + + DETAILS = { locale: [:en], formats: [:html], variants: [], handlers: [:erb] }.freeze + + def find_all(resolver, name = "hello_world", prefix = "test", partial = false, locals = []) + resolver.find_all(name, prefix, partial, DETAILS, nil, locals) + end + + def test_eager_load_templates_populates_cache_without_freezing + with_file "test/hello_world.html.erb", "Hello!" + r = resolver + r.eager_load_templates + + assert_not r.frozen? + templates = find_all(r) + assert_equal 1, templates.size + assert_equal "Hello!", templates[0].source + end + + def test_eager_loaded_resolver_still_binds_new_locals + with_file "test/hello_world.html.erb", "<%= message %>" + r = resolver + r.eager_load_templates + + a = find_all(r, "hello_world", "test", false, [:message])[0] + b = find_all(r, "hello_world", "test", false, [:message, :other])[0] + + assert_not_same a, b + assert_not r.frozen? + end + + def test_freeze_after_eager_load_makes_resolver_shareable + with_file "test/hello_world.html.erb", "<%# locals: () %>Hi" + r = resolver + r.eager_load_templates + r.freeze + + assert_predicate r, :frozen? + assert Ractor.shareable?(r) + + templates = find_all(r) + assert_equal 1, templates.size + assert_predicate templates[0], :frozen? + end + + def test_freeze_raises_for_non_strict_partial + with_file "test/_card.html.erb", "<%= post %>" + r = resolver + r.eager_load_templates + + error = assert_raises(ArgumentError) { r.freeze } + assert_match "test/_card", error.message + assert_match "strict locals", error.message + end + + def test_freeze_raises_for_non_strict_template + with_file "test/hello_world.html.erb", "no locals here" + r = resolver + r.eager_load_templates + + error = assert_raises(ArgumentError) { r.freeze } + assert_match "test/hello_world", error.message + assert_match "strict locals", error.message + end + + def test_frozen_resolver_returns_empty_for_missing_template + with_file "test/hello_world.html.erb", "<%# locals: () %>Hi" + r = resolver + r.eager_load_templates + r.freeze + + assert_empty find_all(r, "nonexistent") + end end