From 3239ed48d28f3c0baf4445e6c279107e892b7cab Mon Sep 17 00:00:00 2001
From: Aaron Patterson <aaron.patterson@gmail.com>
Date: Thu, 18 Feb 2016 08:23:26 -0800
Subject: 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:
https://github.com/rails/rails/blob/13c4cc3b5aea02716b7459c0da641438077f5236/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.
---
actionview/lib/action_view/digestor.rb | 16 +++++++---------
actionview/lib/action_view/lookup_context.rb | 16 ++++++++++++++++
actionview/test/template/digestor_test.rb | 5 ++---
.../test/application/per_request_digest_cache_test.rb | 5 +++--
4 files changed, 28 insertions(+), 14 deletions(-)
diff --git a/actionview/lib/action_view/digestor.rb b/actionview/lib/action_view/digestor.rb
index 657026fa14..f3c5d6c8df 100644
--- a/actionview/lib/action_view/digestor.rb
+++ b/actionview/lib/action_view/digestor.rb
@@ -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
diff --git a/actionview/lib/action_view/lookup_context.rb b/actionview/lib/action_view/lookup_context.rb
index 2a1f4378f5..86afedaa2d 100644
--- a/actionview/lib/action_view/lookup_context.rb
+++ b/actionview/lib/action_view/lookup_context.rb
@@ -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
diff --git a/actionview/test/template/digestor_test.rb b/actionview/test/template/digestor_test.rb
index e50caac6d8..9ff5f7126b 100644
--- a/actionview/test/template/digestor_test.rb
+++ b/actionview/test/template/digestor_test.rb
@@ -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 = {})
diff --git a/railties/test/application/per_request_digest_cache_test.rb b/railties/test/application/per_request_digest_cache_test.rb
index 3198e12662..210646c7c0 100644
--- a/railties/test/application/per_request_digest_cache_test.rb
+++ b/railties/test/application/per_request_digest_cache_test.rb
@@ -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
--
cgit v1.2.3