AR::ConPool - reduce post checkout critical section.

Move post checkout connection verification out of mutex.synchronize.

3 files changed, 38 insertions, 14 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 77e64a22be..aeaaa48945 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
@@ -258,9 +258,7 @@ module ActiveRecord
       def connection
         # 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
+        @reserved_connections[current_connection_id] || synchronized_connection_retrieval
       end
 
       # Is there an open connection that is being used for the current thread?
@@ -342,11 +340,8 @@ module ActiveRecord
       # Raises:
       # - ConnectionTimeoutError: no connection can be obtained from the pool.
       def checkout
-        synchronize do
-          conn = acquire_connection
-          conn.lease
-          checkout_and_verify(conn)
-        end
+        conn = synchronize { acquire_connection.tap(&:lease) }
+        checkout_and_verify(conn)
       end
 
       # Check-in a database connection back into the pool, indicating that you
@@ -377,6 +372,15 @@ module ActiveRecord
 
           release conn, conn.owner
 
+          # @available.any_waiting? => true means that prior to removing this
+          # conn, the pool was at its max size (@connections.size == @size)
+          # this would mean that any threads stuck waiting in the queue wouldn't
+          # know they could checkout_new_connection, so let's do it for them.
+          # Because condition-wait loop is encapsulated in the Queue class
+          # (that in turn is oblivious to ConnectionPool implementation), threads
+          # that are "stuck" there are helpless, they have no way of creating
+          # new connections and are completely reliant on us feeding available
+          # connections into the Queue.
           @available.add checkout_new_connection if @available.any_waiting?
         end
       end
@@ -404,6 +408,17 @@ module ActiveRecord
       end
 
       private
+      def synchronized_connection_retrieval
+        conn = synchronize do
+          # re-checking under lock for correct DCL semantics
+          @reserved_connections[current_connection_id] ||= acquire_connection.tap(&:lease)
+        end
+        begin
+          checkout_and_verify(conn)
+        rescue Exception # clean up if something goes wrong in post_checkout
+          synchronize { @reserved_connections.delete_pair(current_connection_id, conn) }
+        end
+      end
 
       # Acquire a connection by one of 1) immediately removing one
       # from the queue of available connections, 2) creating a new
diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
index 0705c22a8c..60655b673c 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
@@ -7,7 +7,6 @@ require 'active_record/connection_adapters/schema_cache'
 require 'active_record/connection_adapters/sql_type_metadata'
 require 'active_record/connection_adapters/abstract/schema_dumper'
 require 'active_record/connection_adapters/abstract/schema_creation'
-require 'monitor'
 require 'arel/collectors/bind'
 require 'arel/collectors/sql_string'
 
@@ -70,7 +69,6 @@ module ActiveRecord
       include DatabaseLimits
       include QueryCache
       include ActiveSupport::Callbacks
-      include MonitorMixin
       include ColumnDumper
 
       SIMPLE_INT = /\A\d+\z/
@@ -141,12 +139,20 @@ module ActiveRecord
         SchemaCreation.new self
       end
 
+      # this method must only be called while holding connection pool's mutex
       def lease
-        synchronize do
-          unless in_use?
-            @owner = Thread.current
+        if in_use?
+          msg = 'Cannot lease connection, '
+          if @owner == Thread.current
+            msg << 'it is already leased by the current thread.'
+          else
+            msg << "it is already in use by a different thread: #{@owner}. " <<
+                   "Current thread: #{Thread.current}."
           end
+          raise ActiveRecordError, msg
         end
+
+        @owner = Thread.current
       end
 
       def schema_cache=(cache)
@@ -154,6 +160,7 @@ module ActiveRecord
         @schema_cache = cache
       end
 
+      # this method must only be called while holding connection pool's mutex
       def expire
         @owner = nil
       end
diff --git a/activerecord/test/cases/connection_adapters/adapter_leasing_test.rb b/activerecord/test/cases/connection_adapters/adapter_leasing_test.rb
index 662e19f35e..fd5f183ab0 100644
--- a/activerecord/test/cases/connection_adapters/adapter_leasing_test.rb
+++ b/activerecord/test/cases/connection_adapters/adapter_leasing_test.rb
@@ -24,7 +24,9 @@ module ActiveRecord
 
       def test_lease_twice
         assert @adapter.lease, 'should lease adapter'
-        assert_not @adapter.lease, 'should not lease adapter'
+        assert_raises(ActiveRecordError) do
+          @adapter.lease
+        end
       end
 
       def test_expire_mutates_in_use