Permalink
Browse files

Merge pull request #6425 from pinetops/resolver_concurrency_fix

Resolver concurrency fix
  • Loading branch information...
2 parents 513a052 + 719b008 commit 254c04286c5916ae7f91eb6e173b312e7a74e364 @tenderlove tenderlove committed May 21, 2012
Showing with 72 additions and 23 deletions.
  1. +68 −19 actionpack/lib/action_view/template/resolver.rb
  2. +4 −4 actionpack/test/template/lookup_context_test.rb
@@ -2,6 +2,7 @@
require "active_support/core_ext/class"
require "active_support/core_ext/class/attribute_accessors"
require "action_view/template"
+require "thread"
module ActionView
# = Action View Resolver
@@ -24,6 +25,64 @@ def initialize(name, prefix, partial, virtual)
end
end
+ # Threadsafe template cache
+ class Cache #:nodoc:
+ class CacheEntry
+ attr_accessor :templates
+
+ delegate :synchronize, :to => "@mutex"
+
+ def initialize
+ @mutex = Mutex.new
+ end
+ end
+
+ def initialize
+ @data = Hash.new { |h1,k1| h1[k1] = Hash.new { |h2,k2|
+ h2[k2] = Hash.new { |h3,k3| h3[k3] = Hash.new { |h4,k4| h4[k4] = {} } } } }
+ @mutex = Mutex.new
+ end
+
+ # Cache the templates returned by the block
+ def cache(key, name, prefix, partial, locals)
+ cache_entry = nil
+
+ # first obtain a lock on the main data structure to create the cache entry
+ @mutex.synchronize do
+ cache_entry = @data[key][name][prefix][partial][locals] ||= CacheEntry.new
+ end
+
+ # then to avoid a long lasting global lock, obtain a more granular lock
+ # on the CacheEntry itself
+ cache_entry.synchronize do
+ if Resolver.caching?
+ # all templates are cached forever the first time they are accessed
+ cache_entry.templates ||= yield
+ else
+ # templates are still cached, but are only returned if they are
+ # all still current
+ fresh = yield
+
+ mtime = cache_entry.templates && cache_entry.templates.map(&:updated_at).max
+
+ newer = !mtime || fresh.empty? || fresh.any? { |t| t.updated_at > mtime }
+
+ if newer
+ cache_entry.templates = fresh
+ else
+ cache_entry.templates
+ end
+ end
+ end
+ end
+
+ def clear
+ @mutex.synchronize do
+ @data.clear
+ end
+ end
+ end
+
cattr_accessor :caching
self.caching = true
@@ -32,12 +91,11 @@ class << self
end
def initialize
- @cached = Hash.new { |h1,k1| h1[k1] = Hash.new { |h2,k2|
- h2[k2] = Hash.new { |h3,k3| h3[k3] = Hash.new { |h4,k4| h4[k4] = {} } } } }
+ @cache = Cache.new
end
def clear_cache
- @cached.clear
+ @cache.clear
end
# Normalizes the arguments and passes it on to find_template.
@@ -65,27 +123,18 @@ def build_path(name, prefix, partial)
# Handles templates caching. If a key is given and caching is on
# always check the cache before hitting the resolver. Otherwise,
- # it always hits the resolver but check if the resolver is fresher
- # before returning it.
+ # it always hits the resolver but if the key is present, check if the
+ # resolver is fresher before returning it.
def cached(key, path_info, details, locals) #:nodoc:
name, prefix, partial = path_info
locals = locals.map { |x| x.to_s }.sort!
- if key && caching?
- @cached[key][name][prefix][partial][locals] ||= decorate(yield, path_info, details, locals)
- else
- fresh = decorate(yield, path_info, details, locals)
- return fresh unless key
-
- scope = @cached[key][name][prefix][partial]
- cache = scope[locals]
- mtime = cache && cache.map(&:updated_at).max
-
- if !mtime || fresh.empty? || fresh.any? { |t| t.updated_at > mtime }
- scope[locals] = fresh
- else
- cache
+ if key
+ @cache.cache(key, name, prefix, partial, locals) do
+ decorate(yield, path_info, details, locals)
end
+ else
+ decorate(yield, path_info, details, locals)
end
end
@@ -169,7 +169,7 @@ def teardown
assert_not_equal template, old_template
end
-
+
test "responds to #prefixes" do
assert_equal [], @lookup_context.prefixes
@lookup_context.prefixes = ["foo"]
@@ -180,7 +180,7 @@ def teardown
class LookupContextWithFalseCaching < ActiveSupport::TestCase
def setup
@resolver = ActionView::FixtureResolver.new("test/_foo.erb" => ["Foo", Time.utc(2000)])
- @resolver.stubs(:caching?).returns(false)
+ ActionView::Resolver.stubs(:caching?).returns(false)
@lookup_context = ActionView::LookupContext.new(@resolver, {})
end
@@ -247,6 +247,6 @@ def setup
@lookup_context.view_paths.find("foo", "parent", true, details)
end
assert_match %r{Missing partial parent/foo with .* Searched in:\n \* "/Path/to/views"\n}, e.message
- end
-
+ end
+
end

0 comments on commit 254c042

Please sign in to comment.