Merge pull request #8510 from thedarkone/thread_safety_improvements

Thread safety improvements

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'

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 Http
     #   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 @@ def env_filter
       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   = '[^&;=]+'

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 @@ class Dispatcher #:nodoc:
         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 @@ def controller(params, default_controller=true)
       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)

actionpack/lib/action_view/digestor.rb

@@ -1,4 +1,4 @@
-require 'mutex_m'
+require 'thread_safe'
 
 module ActionView
   class Digestor
@@ -21,23 +21,12 @@ class Digestor
     /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 @@ def source
 
       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
 

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 @@ class DetailsKey #:nodoc:
       alias :object_hash :hash
 
       attr_reader :hash
-      @details_keys = Hash.new
+      @details_keys = ThreadSafe::Cache.new
 
       def self.get(details)
         @details_keys[details] ||= new

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

actionpack/lib/action_view/template/resolver.rb

@@ -3,7 +3,7 @@
 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 @@ def to_str
 
     # 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

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 @@ def instance_method_already_implemented?(method_name) #:nodoc:
         # 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
 

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 @@ def initialize(spec)
 
         @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 @@ def initialize(spec)
         # 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 @@ def insert_connection_for_test!(c) #:nodoc:
       # #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 @@ def connected?
       # 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 @@ def disconnect!
       # 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 @@ def checkout_and_verify(c)
     # 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

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,42 +74,35 @@ def method_missing(method, *args, &block)
     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
         relation.__send__(:initialize, klass, *args)
         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)