mirror of
https://github.com/rails/rails.git
synced 2022-11-09 12:12:34 -05:00
move digest cache on to the DetailsKey object
This moves digest calculation cache on to the details key object.
Before, the digest cache was a class level ivar, and one of the keys was
the hash value of the details key object:
13c4cc3b5a/actionview/lib/action_view/digestor.rb (L28)
An object's hash value is not unique, so it's possible for this cache
key to produce colliding keys with no resolution. This commit move
cache on to the details key object itself, so we know that the digests
are always unique per details key object.
This commit is contained in:
parent
13c4cc3b5a
commit
3239ed48d2
4 changed files with 28 additions and 14 deletions
|
@ -4,13 +4,11 @@ require 'monitor'
|
|||
|
||||
module ActionView
|
||||
class Digestor
|
||||
cattr_reader(:cache)
|
||||
@@cache = Concurrent::Map.new
|
||||
@@digest_monitor = Monitor.new
|
||||
|
||||
class PerRequestDigestCacheExpiry < Struct.new(:app) # :nodoc:
|
||||
def call(env)
|
||||
ActionView::Digestor.cache.clear
|
||||
ActionView::LookupContext::DetailsKey.clear
|
||||
app.call(env)
|
||||
end
|
||||
end
|
||||
|
@ -25,12 +23,12 @@ module ActionView
|
|||
def digest(name:, finder:, **options)
|
||||
options.assert_valid_keys(:dependencies, :partial)
|
||||
|
||||
cache_key = ([ name, finder.details_key.hash ].compact + Array.wrap(options[:dependencies])).join('.')
|
||||
cache_key = ([ name ].compact + Array.wrap(options[:dependencies])).join('.')
|
||||
|
||||
# this is a correctly done double-checked locking idiom
|
||||
# (Concurrent::Map's lookups have volatile semantics)
|
||||
@@cache[cache_key] || @@digest_monitor.synchronize do
|
||||
@@cache.fetch(cache_key) do # re-check under lock
|
||||
finder.digest_cache[cache_key] || @@digest_monitor.synchronize do
|
||||
finder.digest_cache.fetch(cache_key) do # re-check under lock
|
||||
compute_and_store_digest(cache_key, name, finder, options)
|
||||
end
|
||||
end
|
||||
|
@ -42,16 +40,16 @@ module ActionView
|
|||
# Prevent re-entry or else recursive templates will blow the stack.
|
||||
# There is no need to worry about other threads seeing the +false+ value,
|
||||
# as they will then have to wait for this thread to let go of the @@digest_monitor lock.
|
||||
pre_stored = @@cache.put_if_absent(cache_key, false).nil? # put_if_absent returns nil on insertion
|
||||
pre_stored = finder.digest_cache.put_if_absent(cache_key, false).nil? # put_if_absent returns nil on insertion
|
||||
PartialDigestor
|
||||
else
|
||||
Digestor
|
||||
end
|
||||
|
||||
@@cache[cache_key] = stored_digest = klass.new(name, finder, options).digest
|
||||
finder.digest_cache[cache_key] = stored_digest = klass.new(name, finder, options).digest
|
||||
ensure
|
||||
# something went wrong or ActionView::Resolver.caching? is false, make sure not to corrupt the @@cache
|
||||
@@cache.delete_pair(cache_key, false) if pre_stored && !stored_digest
|
||||
finder.digest_cache.delete_pair(cache_key, false) if pre_stored && !stored_digest
|
||||
end
|
||||
end
|
||||
|
||||
|
|
|
@ -69,6 +69,18 @@ module ActionView
|
|||
def self.clear
|
||||
@details_keys.clear
|
||||
end
|
||||
|
||||
def self.empty?; @details_keys.empty?; end
|
||||
|
||||
def self.digest_caches
|
||||
@details_keys.values.map(&:digest_cache)
|
||||
end
|
||||
|
||||
attr_reader :digest_cache
|
||||
|
||||
def initialize
|
||||
@digest_cache = Concurrent::Map.new
|
||||
end
|
||||
end
|
||||
|
||||
# Add caching behavior on top of Details.
|
||||
|
@ -194,6 +206,10 @@ module ActionView
|
|||
self.view_paths = view_paths
|
||||
end
|
||||
|
||||
def digest_cache
|
||||
details_key.digest_cache
|
||||
end
|
||||
|
||||
def initialize_details(target, details)
|
||||
registered_details.each do |k|
|
||||
target[k] = details[k] || Accessors::DEFAULT_PROCS[k].call
|
||||
|
|
|
@ -33,7 +33,6 @@ class TemplateDigestorTest < ActionView::TestCase
|
|||
def teardown
|
||||
Dir.chdir @cwd
|
||||
FileUtils.rm_r @tmp_dir
|
||||
ActionView::Digestor.cache.clear
|
||||
end
|
||||
|
||||
def test_top_level_change_reflected
|
||||
|
@ -301,12 +300,12 @@ class TemplateDigestorTest < ActionView::TestCase
|
|||
|
||||
def assert_digest_difference(template_name, options = {})
|
||||
previous_digest = digest(template_name, options)
|
||||
ActionView::Digestor.cache.clear
|
||||
finder.digest_cache.clear
|
||||
|
||||
yield
|
||||
|
||||
assert_not_equal previous_digest, digest(template_name, options), "digest didn't change"
|
||||
ActionView::Digestor.cache.clear
|
||||
finder.digest_cache.clear
|
||||
end
|
||||
|
||||
def digest(template_name, options = {})
|
||||
|
|
|
@ -50,12 +50,13 @@ class PerRequestDigestCacheTest < ActiveSupport::TestCase
|
|||
get '/customers'
|
||||
assert_equal 200, last_response.status
|
||||
|
||||
assert_equal [ '8ba099b7749542fe765ff34a6824d548' ], ActionView::Digestor.cache.values
|
||||
values = ActionView::LookupContext::DetailsKey.digest_caches.first.values
|
||||
assert_equal [ '8ba099b7749542fe765ff34a6824d548' ], values
|
||||
assert_equal %w(david dingus), last_response.body.split.map(&:strip)
|
||||
end
|
||||
|
||||
test "template digests are cleared before a request" do
|
||||
assert_called(ActionView::Digestor.cache, :clear) do
|
||||
assert_called(ActionView::LookupContext::DetailsKey, :clear) do
|
||||
get '/customers'
|
||||
assert_equal 200, last_response.status
|
||||
end
|
||||
|
|
Loading…
Reference in a new issue