Skip to content
This repository
Browse code

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.
  • Loading branch information...
commit 685192bbcba7f887236ddf43ebb7d7dcf7409bd9 1 parent b23ac93
pinetops authored May 20, 2012
69  actionpack/lib/action_view/template/resolver.rb
@@ -2,6 +2,7 @@
2 2
 require "active_support/core_ext/class"
3 3
 require "active_support/core_ext/class/attribute_accessors"
4 4
 require "action_view/template"
  5
+require "thread"
5 6
 
6 7
 module ActionView
7 8
   # = Action View Resolver
@@ -24,6 +25,46 @@ def initialize(name, prefix, partial, virtual)
24 25
       end
25 26
     end
26 27
 
  28
+    # Threadsafe template cache
  29
+    class Cache #:nodoc:
  30
+      def initialize
  31
+        @data = Hash.new { |h1,k1| h1[k1] = Hash.new { |h2,k2|
  32
+                  h2[k2] = Hash.new { |h3,k3| h3[k3] = Hash.new { |h4,k4| h4[k4] = {} } } } }
  33
+        @mutex = Mutex.new
  34
+      end
  35
+
  36
+      # Cache the templates returned by the block
  37
+      def cache(key, name, prefix, partial, locals)
  38
+        @mutex.synchronize do
  39
+          if Resolver.caching?
  40
+            # all templates are cached forever the first time they are accessed
  41
+            @data[key][name][prefix][partial][locals] ||= yield
  42
+          else
  43
+            # templates are still cached, but are only returned if they are
  44
+            # all still current
  45
+            fresh = yield
  46
+
  47
+            cache = @data[key][name][prefix][partial][locals]
  48
+            mtime = cache && cache.map(&:updated_at).max
  49
+
  50
+            newer = !mtime || fresh.empty?  || fresh.any? { |t| t.updated_at > mtime }
  51
+
  52
+            if newer
  53
+              @data[key][name][prefix][partial][locals] = fresh
  54
+            else
  55
+              @data[key][name][prefix][partial][locals]
  56
+            end
  57
+          end
  58
+        end
  59
+      end
  60
+
  61
+      def clear
  62
+        @mutex.synchronize do
  63
+          @data.clear
  64
+        end
  65
+      end
  66
+    end
  67
+
27 68
     cattr_accessor :caching
28 69
     self.caching = true
29 70
 
@@ -32,12 +73,11 @@ class << self
32 73
     end
33 74
 
34 75
     def initialize
35  
-      @cached = Hash.new { |h1,k1| h1[k1] = Hash.new { |h2,k2|
36  
-        h2[k2] = Hash.new { |h3,k3| h3[k3] = Hash.new { |h4,k4| h4[k4] = {} } } } }
  76
+      @cache = Cache.new
37 77
     end
38 78
 
39 79
     def clear_cache
40  
-      @cached.clear
  80
+      @cache.clear
41 81
     end
42 82
 
43 83
     # Normalizes the arguments and passes it on to find_template.
@@ -65,27 +105,18 @@ def build_path(name, prefix, partial)
65 105
 
66 106
     # Handles templates caching. If a key is given and caching is on
67 107
     # always check the cache before hitting the resolver. Otherwise,
68  
-    # it always hits the resolver but check if the resolver is fresher
69  
-    # before returning it.
  108
+    # it always hits the resolver but if the key is present, check if the
  109
+    # resolver is fresher before returning it.
70 110
     def cached(key, path_info, details, locals) #:nodoc:
71 111
       name, prefix, partial = path_info
72 112
       locals = locals.map { |x| x.to_s }.sort!
73 113
 
74  
-      if key && caching?
75  
-        @cached[key][name][prefix][partial][locals] ||= decorate(yield, path_info, details, locals)
76  
-      else
77  
-        fresh = decorate(yield, path_info, details, locals)
78  
-        return fresh unless key
79  
-
80  
-        scope = @cached[key][name][prefix][partial]
81  
-        cache = scope[locals]
82  
-        mtime = cache && cache.map(&:updated_at).max
83  
-
84  
-        if !mtime || fresh.empty?  || fresh.any? { |t| t.updated_at > mtime }
85  
-          scope[locals] = fresh
86  
-        else
87  
-          cache
  114
+      if key
  115
+        @cache.cache(key, name, prefix, partial, locals) do
  116
+          decorate(yield, path_info, details, locals)
88 117
         end
  118
+      else
  119
+        decorate(yield, path_info, details, locals)
89 120
       end
90 121
     end
91 122
 
8  actionpack/test/template/lookup_context_test.rb
@@ -169,7 +169,7 @@ def teardown
169 169
 
170 170
     assert_not_equal template, old_template
171 171
   end
172  
-  
  172
+
173 173
   test "responds to #prefixes" do
174 174
     assert_equal [], @lookup_context.prefixes
175 175
     @lookup_context.prefixes = ["foo"]
@@ -180,7 +180,7 @@ def teardown
180 180
 class LookupContextWithFalseCaching < ActiveSupport::TestCase
181 181
   def setup
182 182
     @resolver = ActionView::FixtureResolver.new("test/_foo.erb" => ["Foo", Time.utc(2000)])
183  
-    @resolver.stubs(:caching?).returns(false)
  183
+    ActionView::Resolver.stubs(:caching?).returns(false)
184 184
     @lookup_context = ActionView::LookupContext.new(@resolver, {})
185 185
   end
186 186
 
@@ -247,6 +247,6 @@ def setup
247 247
       @lookup_context.view_paths.find("foo", "parent", true, details)
248 248
     end
249 249
     assert_match %r{Missing partial parent/foo with .* Searched in:\n  \* "/Path/to/views"\n}, e.message
250  
-  end 
251  
-  
  250
+  end
  251
+
252 252
 end

0 notes on commit 685192b

Please sign in to comment.
Something went wrong with that request. Please try again.