Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Resolver concurrency fix #6425

Merged
merged 2 commits into from

2 participants

pinetops Aaron Patterson
pinetops

Replacement for #6412

This is a fix for #6404

The Template cache in the Resolver can be accessed by multiple threads
similtaneously in multi-threaded environments. The cache is implemented
using a Hash, which isn't threadsafe in all VMs (notably JRuby).

This includes two commits, the first puts a global lock around the whole process.
The second improves this to add a more granular lock.

pinetops added some commits
pinetops pinetops Make the Resolver template cache threadsafe - closes #6404
The Template cache in the Resolver can be accessed by multiple threads
similtaneously in multi-threaded environments. The cache is implemented
using a Hash, which isn't threadsafe in all VMs (notably JRuby).

This commit extracts the cache to a new Cache class and adds mutexes to
prevent concurrent access.
685192b
pinetops pinetops More granular locking of the Resolver template cache
In order to avoid holding a global lock when doing template
resolution, instead add individual locks on a per cache entry
basis. The global lock is now only used for manipulation of the main
cache data structure.
719b008
Aaron Patterson tenderlove merged commit 254c042 into from
José Valim josevalim referenced this pull request from a commit
José Valim josevalim Revert "Merge pull request #6425 from pinetops/resolver_concurrency_fix"
This reverts commit 254c042, reversing
changes made to 513a052.
776ea10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 21, 2012
  1. pinetops

    Make the Resolver template cache threadsafe - closes #6404

    pinetops authored
    The Template cache in the Resolver can be accessed by multiple threads
    similtaneously in multi-threaded environments. The cache is implemented
    using a Hash, which isn't threadsafe in all VMs (notably JRuby).
    
    This commit extracts the cache to a new Cache class and adds mutexes to
    prevent concurrent access.
  2. pinetops

    More granular locking of the Resolver template cache

    pinetops authored
    In order to avoid holding a global lock when doing template
    resolution, instead add individual locks on a per cache entry
    basis. The global lock is now only used for manipulation of the main
    cache data structure.
This page is out of date. Refresh to see the latest.
87 actionpack/lib/action_view/template/resolver.rb
View
@@ -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
8 actionpack/test/template/lookup_context_test.rb
View
@@ -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
Something went wrong with that request. Please try again.