Merge pull request #8510 from thedarkone/thread_safety_improvements

Thread safety improvements

2 files changed, 32 insertions, 31 deletions

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)