Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ nav_order: 6

## main

* Invalidate Action View's memoized template digests when a component registers with `ViewComponent::CacheDigest`, so a digest computed before the component loaded isn't served for the rest of the process.

*Erik Axel Nielsen*

* Raise errors encountered while computing a component's cache digest instead of degrading to an untracked component that can serve stale fragments.

*Erik Axel Nielsen*
Expand Down
15 changes: 15 additions & 0 deletions lib/view_component/cache_digest.rb
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,16 @@ def enabled?
# @private
def register(component)
return unless component.virtual_path && component.name
return if registry[component.virtual_path] == component.name

registry[component.virtual_path] = component.name

# Components register as they load, and under lazy loading that happens
# after Action View has already digested and memoized some templates.
# Those digests were computed without this component's dependencies and
# would otherwise be served for the rest of the process, making the
# digest a function of load order rather than of source.
expire_digests
end

# The synthetic virtual path a component is digested under.
Expand Down Expand Up @@ -220,6 +228,13 @@ def install!

private

# Drop Action View's memoized template digests, leaving its resolver
# caches alone: no template changed, only the set of dependencies the
# Digestor can see.
def expire_digests
ActionView::LookupContext::DetailsKey.digest_caches.each(&:clear)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I don't think this should be fixed:

  • It does not affect production, since it eager-loads
  • For development the remaining race needs two concurrent requests during the very first cacheable-component load. A stale entry then lives until Action View next clears DetailsKey (a change under a view path) or the process restarts.

Below is a longer explanation of what we would need to do to fix it. Let me know what you decide.

When it can produce a wrong digest. Only while the feature is switching on: the first cacheable component registering while another thread is mid-digest. Once the registry is non-empty, DependencyTracking scans every template, and constantize_component autoloads any component it finds before returning its path. By the time a path is in the tree its component has registered, so an in-flight digest that straddles a later registration is already correct, and memoizing it after the clear is harmless.

Why not take the Digestor’s lock. register runs from included/inherited, during an autoload, while Ruby holds the require lock for that file. A digesting thread holds the digest mutex and, once the tracker is installed, may safe_constantize the same component and block on that require lock, so the two threads wait on each other. On a single thread, the scan’s own autoload re-enters register while the mutex is already held, which raises ThreadError. The mutex is also private and has changed shape (@@digest_mutex through 8.1, a Ractor-local digest_mutex on main).

Closing it fully would mean wrapping ActionView::Digestor.digest with a registry generation check and retry, installed at boot for every app rather than on the first include. A thread already inside digest when the first component loads would never pass through a wrapper installed later. That runs against the current design, where nothing is installed until a component opts in, so I’ve left it out. Happy to add it if you’d prefer the stronger guarantee.

end

# Resolve a constant name to a component that opted into caching.
#
# Returns nil for anything else, including constants that don't exist.
Expand Down
31 changes: 31 additions & 0 deletions test/sandbox/test/experimentally_cacheable_integration_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,28 @@ def test_fragment_inside_a_component_template_is_unaffected_by_unrelated_compone
end
end

# Components register as they're autoloaded, so under lazy loading a template
# can be digested before the components it renders have loaded. Action View
# memoizes digests for the life of the process, so that first digest sticks:
# without invalidation the same source digests differently depending on the
# order things happened to load in.
def test_digest_does_not_depend_on_when_the_component_registered
registered_first = with_registry("cacheable_component" => "CacheableComponent") do
clear_digest_cache
fragment_digest_for("integration_examples/cached_component")
end

registered_late = with_registry({}) do
clear_digest_cache
fragment_digest_for("integration_examples/cached_component")
ViewComponent::CacheDigest.register(CacheableComponent)

fragment_digest_for("integration_examples/cached_component")
end

assert_equal registered_first, registered_late
end

def test_component_output_is_cached_between_requests
get "/cached_component"
assert_select(".cacheable", text: "cached")
Expand Down Expand Up @@ -158,6 +180,15 @@ def fragment_key_for(path)
capture_fragment_key { with_new_cache { get path } }
end

def with_registry(entries)
saved = ViewComponent::CacheDigest.registry.dup
ViewComponent::CacheDigest.registry.replace(entries)
yield
ensure
ViewComponent::CacheDigest.registry.replace(saved)
clear_digest_cache
end

def view_context
ApplicationController.new.tap { |c| c.request = ActionDispatch::TestRequest.create }.view_context
end
Expand Down
31 changes: 31 additions & 0 deletions test/sandbox/test/experimentally_cacheable_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,32 @@ def test_registers_component_with_the_digest_registry
)
end

# Registration runs on every class load, so an unchanged component must not
# throw away digests other templates are still using.
def test_registering_an_unchanged_component_leaves_memoized_digests_alone
clear_digest_cache
CacheableComponent.cache_digest
memoized = digest_cache_size

assert_operator memoized, :>, 0

ViewComponent::CacheDigest.register(CacheableComponent)

assert_equal memoized, digest_cache_size
end

def test_registering_a_new_component_expires_memoized_digests
clear_digest_cache
CacheableComponent.cache_digest

assert_operator digest_cache_size, :>, 0

ViewComponent::CacheDigest.registry.delete("cacheable_component")
ViewComponent::CacheDigest.register(CacheableComponent)

assert_equal 0, digest_cache_size
end

def test_component_is_marked_cacheable
assert_predicate CacheableComponent, :__vc_cacheable?
refute_respond_to ErbComponent, :__vc_cacheable?
Expand Down Expand Up @@ -513,6 +539,11 @@ def recompile(component)
component.__vc_compile(force: true)
end

# Every digest Action View has memoized, across all details keys.
def digest_cache_size
ActionView::LookupContext::DetailsKey.digest_caches.sum(&:size)
end

def build_template(source, virtual_path: "test/template")
ActionView::Template.new(
source,
Expand Down
Loading