Merge pull request #8510 from thedarkone/thread_safety_improvements

Thread safety improvements

15 files changed, 101 insertions, 108 deletions

diff --git a/Gemfile b/Gemfile
index 35bcb68fb2..8be17a841f 100644
--- a/Gemfile
+++ b/Gemfile
@@ -12,6 +12,7 @@ gem 'jquery-rails', '~> 2.1.4', github: 'rails/jquery-rails'
 gem 'turbolinks'
 gem 'coffee-rails', github: 'rails/coffee-rails'
 
+gem 'thread_safe', '~> 0.1'
 gem 'journey', github: 'rails/journey', branch: 'master'
 
 gem 'activerecord-deprecated_finders', github: 'rails/activerecord-deprecated_finders', branch: 'master'
diff --git a/actionpack/lib/action_dispatch/http/filter_parameters.rb b/actionpack/lib/action_dispatch/http/filter_parameters.rb
index 4a7df6b657..4fea690862 100644
--- a/actionpack/lib/action_dispatch/http/filter_parameters.rb
+++ b/actionpack/lib/action_dispatch/http/filter_parameters.rb
@@ -1,4 +1,4 @@
-require 'mutex_m'
+require 'thread_safe'
 require 'active_support/core_ext/hash/keys'
 require 'active_support/core_ext/object/duplicable'
 
@@ -21,7 +21,7 @@ module ActionDispatch
     #   end
     #   => reverses the value to all keys matching /secret/i
     module FilterParameters
-      @@parameter_filter_for  = {}.extend(Mutex_m)
+      @@parameter_filter_for = ThreadSafe::Cache.new
 
       ENV_MATCH = [/RAW_POST_DATA/, "rack.request.form_vars"] # :nodoc:
       NULL_PARAM_FILTER = ParameterFilter.new # :nodoc:
@@ -65,11 +65,7 @@ module ActionDispatch
       end
 
       def parameter_filter_for(filters)
-        @@parameter_filter_for.synchronize do
-          # Do we *actually* need this cache? Constructing ParameterFilters
-          # doesn't seem too expensive.
-          @@parameter_filter_for[filters] ||= ParameterFilter.new(filters)
-        end
+        @@parameter_filter_for[filters] ||= ParameterFilter.new(filters)
       end
 
       KV_RE   = '[^&;=]+'
diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb
index c11e66d110..eb9d4b24f1 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -1,5 +1,6 @@
 require 'journey'
 require 'forwardable'
+require 'thread_safe'
 require 'active_support/core_ext/object/to_query'
 require 'active_support/core_ext/hash/slice'
 require 'active_support/core_ext/module/remove_method'
@@ -20,7 +21,7 @@ module ActionDispatch
         def initialize(options={})
           @defaults = options[:defaults]
           @glob_param = options.delete(:glob)
-          @controllers = {}
+          @controller_class_names = ThreadSafe::Cache.new
         end
 
         def call(env)
@@ -68,13 +69,8 @@ module ActionDispatch
       private
 
         def controller_reference(controller_param)
-          controller_name = "#{controller_param.camelize}Controller"
-
-          unless controller = @controllers[controller_param]
-            controller = @controllers[controller_param] =
-              ActiveSupport::Dependencies.reference(controller_name)
-          end
-          controller.get(controller_name)
+          const_name = @controller_class_names[controller_param] ||= "#{controller_param.camelize}Controller"
+          ActiveSupport::Dependencies.constantize(const_name)
         end
 
         def dispatch(controller, action, env)
diff --git a/actionpack/lib/action_view/digestor.rb b/actionpack/lib/action_view/digestor.rb
index 1c6eaf36f7..8bc69b9246 100644
--- a/actionpack/lib/action_view/digestor.rb
+++ b/actionpack/lib/action_view/digestor.rb
@@ -1,4 +1,4 @@
-require 'mutex_m'
+require 'thread_safe'
 
 module ActionView
   class Digestor
@@ -21,23 +21,12 @@ module ActionView
     /x
 
     cattr_reader(:cache)
-    @@cache = Hash.new.extend Mutex_m
+    @@cache = ThreadSafe::Cache.new
 
     def self.digest(name, format, finder, options = {})
-      cache.synchronize do
-        unsafe_digest name, format, finder, options
-      end
-    end
-
-    ###
-    # This method is NOT thread safe.  DO NOT CALL IT DIRECTLY, instead call
-    # Digestor.digest
-    def self.unsafe_digest(name, format, finder, options = {}) # :nodoc:
-      key = "#{name}.#{format}"
-
-      cache.fetch(key) do
+      @@cache["#{name}.#{format}"] ||= begin
         klass = options[:partial] || name.include?("/_") ? PartialDigestor : Digestor
-        cache[key] = klass.new(name, format, finder).digest
+        klass.new(name, format, finder).digest
       end
     end
 
@@ -93,7 +82,7 @@ module ActionView
 
       def dependency_digest
         dependencies.collect do |template_name|
-          Digestor.unsafe_digest(template_name, format, finder, partial: true)
+          Digestor.digest(template_name, format, finder, partial: true)
         end.join("-")
       end
 
diff --git a/actionpack/lib/action_view/lookup_context.rb b/actionpack/lib/action_view/lookup_context.rb
index 76f4dea7b8..4e4816d983 100644
--- a/actionpack/lib/action_view/lookup_context.rb
+++ b/actionpack/lib/action_view/lookup_context.rb
@@ -1,3 +1,4 @@
+require 'thread_safe'
 require 'active_support/core_ext/module/remove_method'
 
 module ActionView
@@ -51,7 +52,7 @@ module ActionView
       alias :object_hash :hash
 
       attr_reader :hash
-      @details_keys = Hash.new
+      @details_keys = ThreadSafe::Cache.new
 
       def self.get(details)
         @details_keys[details] ||= new
diff --git a/actionpack/lib/action_view/renderer/partial_renderer.rb b/actionpack/lib/action_view/renderer/partial_renderer.rb
index 8fb9b6ff18..37f93a13fc 100644
--- a/actionpack/lib/action_view/renderer/partial_renderer.rb
+++ b/actionpack/lib/action_view/renderer/partial_renderer.rb
@@ -1,3 +1,5 @@
+require 'thread_safe'
+
 module ActionView
   # = Action View Partials
   #
@@ -247,7 +249,9 @@ module ActionView
   #     <%- end -%>
   #   <% end %>
   class PartialRenderer < AbstractRenderer
-    PREFIXED_PARTIAL_NAMES = Hash.new { |h,k| h[k] = {} }
+    PREFIXED_PARTIAL_NAMES = ThreadSafe::Cache.new do |h, k|
+      h[k] = ThreadSafe::Cache.new
+    end
 
     def initialize(*)
       super
diff --git a/actionpack/lib/action_view/template/resolver.rb b/actionpack/lib/action_view/template/resolver.rb
index fc77c1485d..8b23029bbc 100644
--- a/actionpack/lib/action_view/template/resolver.rb
+++ b/actionpack/lib/action_view/template/resolver.rb
@@ -3,7 +3,7 @@ require "active_support/core_ext/class"
 require "active_support/core_ext/class/attribute_accessors"
 require "action_view/template"
 require "thread"
-require "mutex_m"
+require "thread_safe"
 
 module ActionView
   # = Action View Resolver
@@ -35,52 +35,51 @@ module ActionView
 
     # Threadsafe template cache
     class Cache #:nodoc:
-      class CacheEntry
-        include Mutex_m
-
-        attr_accessor :templates
+      class SmallCache < ThreadSafe::Cache
+        def initialize(options = {})
+          super(options.merge(:initial_capacity => 2))
+        end
       end
 
+      # preallocate all the default blocks for performance/memory consumption reasons
+      PARTIAL_BLOCK = lambda {|cache, partial| cache[partial] = SmallCache.new}
+      PREFIX_BLOCK  = lambda {|cache, prefix|  cache[prefix]  = SmallCache.new(&PARTIAL_BLOCK)}
+      NAME_BLOCK    = lambda {|cache, name|    cache[name]    = SmallCache.new(&PREFIX_BLOCK)}
+      KEY_BLOCK     = lambda {|cache, key|     cache[key]     = SmallCache.new(&NAME_BLOCK)}
+
+      # usually a majority of template look ups return nothing, use this canonical preallocated array to safe memory
+      NO_TEMPLATES = [].freeze
+
       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
+        @data = SmallCache.new(&KEY_BLOCK)
       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?
-            cache_entry.templates ||= yield
+        if Resolver.caching?
+          @data[key][name][prefix][partial][locals] ||= canonical_no_templates(yield)
+        else
+          fresh_templates  = yield
+          cached_templates = @data[key][name][prefix][partial][locals]
+
+          if templates_have_changed?(cached_templates, fresh_templates)
+            @data[key][name][prefix][partial][locals] = canonical_no_templates(fresh_templates)
           else
-            fresh_templates = yield
-
-            if templates_have_changed?(cache_entry.templates, fresh_templates)
-              cache_entry.templates = fresh_templates
-            else
-              cache_entry.templates ||= []
-            end
+            cached_templates || NO_TEMPLATES
           end
         end
       end
 
       def clear
-        @mutex.synchronize do
-          @data.clear
-        end
+        @data.clear
       end
 
       private
 
+      def canonical_no_templates(templates)
+        templates.empty? ? NO_TEMPLATES : templates
+      end
+
       def templates_have_changed?(cached_templates, fresh_templates)
         # if either the old or new template list is empty, we don't need to (and can't)
         # compare modification times, and instead just check whether the lists are different
diff --git a/activemodel/lib/active_model/attribute_methods.rb b/activemodel/lib/active_model/attribute_methods.rb
index af11da1351..db5759ada9 100644
--- a/activemodel/lib/active_model/attribute_methods.rb
+++ b/activemodel/lib/active_model/attribute_methods.rb
@@ -1,3 +1,4 @@
+require 'thread_safe'
 
 module ActiveModel
   # Raised when an attribute is not defined.
@@ -337,17 +338,17 @@ module ActiveModel
         # significantly (in our case our test suite finishes 10% faster with
         # this cache).
         def attribute_method_matchers_cache #:nodoc:
-          @attribute_method_matchers_cache ||= {}
+          @attribute_method_matchers_cache ||= ThreadSafe::Cache.new(:initial_capacity => 4)
         end
 
         def attribute_method_matcher(method_name) #:nodoc:
-          attribute_method_matchers_cache.fetch(method_name) do |name|
+          attribute_method_matchers_cache.compute_if_absent(method_name) do
             # Must try to match prefixes/suffixes first, or else the matcher with no prefix/suffix
             # will match every time.
             matchers = attribute_method_matchers.partition(&:plain?).reverse.flatten(1)
             match = nil
-            matchers.detect { |method| match = method.match(name) }
-            attribute_method_matchers_cache[name] = match
+            matchers.detect { |method| match = method.match(method_name) }
+            match
           end
         end
 
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
index b5a8011ca4..82d0cf7e2e 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
@@ -1,4 +1,5 @@
 require 'thread'
+require 'thread_safe'
 require 'monitor'
 require 'set'
 require 'active_support/deprecation'
@@ -236,9 +237,6 @@ module ActiveRecord
 
         @spec = spec
 
-        # The cache of reserved connections mapped to threads
-        @reserved_connections = {}
-
         @checkout_timeout = spec.config[:checkout_timeout] || 5
         @dead_connection_timeout = spec.config[:dead_connection_timeout]
         @reaper  = Reaper.new self, spec.config[:reaping_frequency]
@@ -247,6 +245,9 @@ module ActiveRecord
         # default max pool size to 5
         @size = (spec.config[:pool] && spec.config[:pool].to_i) || 5
 
+        # The cache of reserved connections mapped to threads
+        @reserved_connections = ThreadSafe::Cache.new(:initial_capacity => @size)
+
         @connections         = []
         @automatic_reconnect = true
 
@@ -267,7 +268,9 @@ module ActiveRecord
       # #connection can be called any number of times; the connection is
       # held in a hash keyed by the thread id.
       def connection
-        synchronize do
+        # this is correctly done double-checked locking
+        # (ThreadSafe::Cache's lookups have volatile semantics)
+        @reserved_connections[current_connection_id] || synchronize do
           @reserved_connections[current_connection_id] ||= checkout
         end
       end
@@ -310,7 +313,7 @@ module ActiveRecord
       # Disconnects all connections in the pool, and clears the pool.
       def disconnect!
         synchronize do
-          @reserved_connections = {}
+          @reserved_connections.clear
           @connections.each do |conn|
             checkin conn
             conn.disconnect!
@@ -323,7 +326,7 @@ module ActiveRecord
       # Clears the cache which maps classes.
       def clear_reloadable_connections!
         synchronize do
-          @reserved_connections = {}
+          @reserved_connections.clear
           @connections.each do |conn|
             checkin conn
             conn.disconnect! if conn.requires_reloading?
@@ -490,11 +493,15 @@ module ActiveRecord
     # determine the connection pool that they should use.
     class ConnectionHandler
       def initialize
-        # These hashes are keyed by klass.name, NOT klass. Keying them by klass
+        # These caches are keyed by klass.name, NOT klass. Keying them by klass
         # alone would lead to memory leaks in development mode as all previous
         # instances of the class would stay in memory.
-        @owner_to_pool = Hash.new { |h,k| h[k] = {} }
-        @class_to_pool = Hash.new { |h,k| h[k] = {} }
+        @owner_to_pool = ThreadSafe::Cache.new(:initial_capacity => 2) do |h,k|
+          h[k] = ThreadSafe::Cache.new(:initial_capacity => 2)
+        end
+        @class_to_pool = ThreadSafe::Cache.new(:initial_capacity => 2) do |h,k|
+          h[k] = ThreadSafe::Cache.new
+        end
       end
 
       def connection_pool_list
diff --git a/activerecord/lib/active_record/relation/delegation.rb b/activerecord/lib/active_record/relation/delegation.rb
index 2184625e22..431d083f21 100644
--- a/activerecord/lib/active_record/relation/delegation.rb
+++ b/activerecord/lib/active_record/relation/delegation.rb
@@ -1,5 +1,6 @@
 require 'active_support/concern'
-require 'mutex_m'
+require 'thread'
+require 'thread_safe'
 
 module ActiveRecord
   module Delegation # :nodoc:
@@ -73,8 +74,7 @@ module ActiveRecord
     end
 
     module ClassMethods
-      # This hash is keyed by klass.name to avoid memory leaks in development mode
-      @@subclasses = Hash.new { |h, k| h[k] = {} }.extend(Mutex_m)
+      @@subclasses = ThreadSafe::Cache.new(:initial_capacity => 2)
 
       def new(klass, *args)
         relation = relation_class_for(klass).allocate
@@ -82,33 +82,27 @@ module ActiveRecord
         relation
       end
 
+      # This doesn't have to be thread-safe. relation_class_for guarantees that this will only be
+      # called exactly once for a given const name.
+      def const_missing(name)
+        const_set(name, Class.new(self) { include ClassSpecificRelation })
+      end
+
+      private
       # Cache the constants in @@subclasses because looking them up via const_get
       # make instantiation significantly slower.
       def relation_class_for(klass)
-        if klass && klass.name
-          if subclass = @@subclasses.synchronize { @@subclasses[self][klass.name] }
-            subclass
-          else
-            subclass = const_get("#{name.gsub('::', '_')}_#{klass.name.gsub('::', '_')}", false)
-            @@subclasses.synchronize { @@subclasses[self][klass.name] = subclass }
-            subclass
+        if klass && (klass_name = klass.name)
+          my_cache = @@subclasses.compute_if_absent(self) { ThreadSafe::Cache.new }
+          # This hash is keyed by klass.name to avoid memory leaks in development mode
+          my_cache.compute_if_absent(klass_name) do
+            # Cache#compute_if_absent guarantees that the block will only executed once for the given klass_name
+            const_get("#{name.gsub('::', '_')}_#{klass_name.gsub('::', '_')}", false)
           end
         else
           ActiveRecord::Relation
         end
       end
-
-      # Check const_defined? in case another thread has already defined the constant.
-      # I am not sure whether this is strictly necessary.
-      def const_missing(name)
-        @@subclasses.synchronize {
-          if const_defined?(name)
-            const_get(name)
-          else
-            const_set(name, Class.new(self) { include ClassSpecificRelation })
-          end
-        }
-      end
     end
 
     def respond_to?(method, include_private = false)
diff --git a/activesupport/activesupport.gemspec b/activesupport/activesupport.gemspec
index a4216d2cb4..4c9e59dbd2 100644
--- a/activesupport/activesupport.gemspec
+++ b/activesupport/activesupport.gemspec
@@ -24,4 +24,5 @@ Gem::Specification.new do |s|
   s.add_dependency 'multi_json', '~> 1.3'
   s.add_dependency 'tzinfo',     '~> 0.3.33'
   s.add_dependency 'minitest',   '~> 4.1'
+  s.add_dependency 'thread_safe','~> 0.1'
 end
diff --git a/activesupport/lib/active_support/dependencies.rb b/activesupport/lib/active_support/dependencies.rb
index efd351d741..fff4c776a9 100644
--- a/activesupport/lib/active_support/dependencies.rb
+++ b/activesupport/lib/active_support/dependencies.rb
@@ -1,5 +1,6 @@
 require 'set'
 require 'thread'
+require 'thread_safe'
 require 'pathname'
 require 'active_support/core_ext/module/aliasing'
 require 'active_support/core_ext/module/attribute_accessors'
@@ -517,7 +518,7 @@ module ActiveSupport #:nodoc:
 
     class ClassCache
       def initialize
-        @store = Hash.new
+        @store = ThreadSafe::Cache.new
       end
 
       def empty?
diff --git a/activesupport/lib/active_support/inflector/inflections.rb b/activesupport/lib/active_support/inflector/inflections.rb
index 6f259a093b..9cf4b2b2ba 100644
--- a/activesupport/lib/active_support/inflector/inflections.rb
+++ b/activesupport/lib/active_support/inflector/inflections.rb
@@ -1,3 +1,4 @@
+require 'thread_safe'
 require 'active_support/core_ext/array/prepend_and_append'
 require 'active_support/i18n'
 
@@ -24,9 +25,10 @@ module ActiveSupport
     # singularization rules that is runs. This guarantees that your rules run
     # before any of the rules that may already have been loaded.
     class Inflections
+      @__instance__ = ThreadSafe::Cache.new
+
       def self.instance(locale = :en)
-        @__instance__ ||= Hash.new { |h, k| h[k] = new }
-        @__instance__[locale]
+        @__instance__[locale] ||= new
       end
 
       attr_reader :plurals, :singulars, :uncountables, :humans, :acronyms, :acronym_regex
diff --git a/activesupport/lib/active_support/key_generator.rb b/activesupport/lib/active_support/key_generator.rb
index 6beb2b6afa..71654dbb87 100644
--- a/activesupport/lib/active_support/key_generator.rb
+++ b/activesupport/lib/active_support/key_generator.rb
@@ -1,4 +1,4 @@
-require 'mutex_m'
+require 'thread_safe'
 require 'openssl'
 
 module ActiveSupport
@@ -28,16 +28,14 @@ module ActiveSupport
   class CachingKeyGenerator
     def initialize(key_generator)
       @key_generator = key_generator
-      @cache_keys = {}.extend(Mutex_m)
+      @cache_keys = ThreadSafe::Cache.new
     end
 
     # Returns a derived key suitable for use.  The default key_size is chosen
     # to be compatible with the default settings of ActiveSupport::MessageVerifier.
     # i.e. OpenSSL::Digest::SHA1#block_length
     def generate_key(salt, key_size=64)
-      @cache_keys.synchronize do
-        @cache_keys["#{salt}#{key_size}"] ||= @key_generator.generate_key(salt, key_size)
-      end
+      @cache_keys["#{salt}#{key_size}"] ||= @key_generator.generate_key(salt, key_size)
     end
   end
 
diff --git a/activesupport/lib/active_support/notifications/fanout.rb b/activesupport/lib/active_support/notifications/fanout.rb
index 2e5bcf4639..7588fdb67c 100644
--- a/activesupport/lib/active_support/notifications/fanout.rb
+++ b/activesupport/lib/active_support/notifications/fanout.rb
@@ -1,4 +1,5 @@
 require 'mutex_m'
+require 'thread_safe'
 
 module ActiveSupport
   module Notifications
@@ -11,7 +12,7 @@ module ActiveSupport
 
       def initialize
         @subscribers = []
-        @listeners_for = {}
+        @listeners_for = ThreadSafe::Cache.new
         super
       end
 
@@ -44,7 +45,9 @@ module ActiveSupport
       end
 
       def listeners_for(name)
-        synchronize do
+        # this is correctly done double-checked locking (ThreadSafe::Cache's lookups have volatile semantics)
+        @listeners_for[name] || synchronize do
+          # use synchronisation when accessing @subscribers
           @listeners_for[name] ||= @subscribers.select { |s| s.subscribed_to?(name) }
         end
       end