Reap connections based on owning-thread death

.. not a general timeout.

Now, if a thread checks out a connection then dies, we can immediately
recover that connection and re-use it.

This should alleviate the pool exhaustion discussed in #12867. More
importantly, it entirely avoids the potential issues of the reaper
attempting to check whether connections are still active: as long as the
owning thread is alive, the connection is its business alone.

As a no-op reap is now trivial (only entails checking a thread status
per connection), we can also perform one in-line any time we decide to
sleep for a connection.

1 files changed, 28 insertions, 28 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 fd185de589..db80c0faee 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
@@ -58,13 +58,11 @@ module ActiveRecord
     # * +checkout_timeout+: number of seconds to block and wait for a connection
     #   before giving up and raising a timeout error (default 5 seconds).
     # * +reaping_frequency+: frequency in seconds to periodically run the
-    #   Reaper, which attempts to find and close dead connections, which can
-    #   occur if a programmer forgets to close a connection at the end of a
-    #   thread or a thread dies unexpectedly. (Default nil, which means don't
-    #   run the Reaper).
-    # * +dead_connection_timeout+: number of seconds from last checkout
-    #   after which the Reaper will consider a connection reapable. (default
-    #   5 seconds).
+    #   Reaper, which attempts to find and recover connections from dead
+    #   threads, which can occur if a programmer forgets to close a
+    #   connection at the end of a thread or a thread dies unexpectedly.
+    #   Regardless of this setting, the Reaper will be invoked before every
+    #   blocking wait. (Default nil, which means don't schedule the Reaper).
     class ConnectionPool
       # Threadsafe, fair, FIFO queue.  Meant to be used by ConnectionPool
       # with which it shares a Monitor.  But could be a generic Queue.
@@ -222,7 +220,7 @@ module ActiveRecord
 
       include MonitorMixin
 
-      attr_accessor :automatic_reconnect, :checkout_timeout, :dead_connection_timeout
+      attr_accessor :automatic_reconnect, :checkout_timeout
       attr_reader :spec, :connections, :size, :reaper
 
       # Creates a new ConnectionPool object. +spec+ is a ConnectionSpecification
@@ -237,7 +235,6 @@ module ActiveRecord
         @spec = spec
 
         @checkout_timeout = spec.config[:checkout_timeout] || 5
-        @dead_connection_timeout = spec.config[:dead_connection_timeout] || 5
         @reaper  = Reaper.new self, spec.config[:reaping_frequency]
         @reaper.run
 
@@ -361,11 +358,13 @@ module ActiveRecord
       # calling +checkout+ on this pool.
       def checkin(conn)
         synchronize do
+          owner = conn.owner
+
           conn.run_callbacks :checkin do
             conn.expire
           end
 
-          release conn
+          release conn, owner
 
           @available.add conn
         end
@@ -378,22 +377,28 @@ module ActiveRecord
           @connections.delete conn
           @available.delete conn
 
-          # FIXME: we might want to store the key on the connection so that removing
-          # from the reserved hash will be a little easier.
-          release conn
+          release conn, conn.owner
 
           @available.add checkout_new_connection if @available.any_waiting?
         end
       end
 
-      # Removes dead connections from the pool.  A dead connection can occur
-      # if a programmer forgets to close a connection at the end of a thread
+      # Recover lost connections for the pool.  A lost connection can occur if
+      # a programmer forgets to checkin a connection at the end of a thread
       # or a thread dies unexpectedly.
       def reap
-        synchronize do
-          stale = Time.now - @dead_connection_timeout
-          connections.dup.each do |conn|
-            if conn.in_use? && stale > conn.last_use && !conn.active_threadsafe?
+        stale_connections = synchronize do
+          @connections.select do |conn|
+            conn.in_use? && !conn.owner.alive?
+          end
+        end
+
+        stale_connections.each do |conn|
+          synchronize do
+            if conn.active?
+              conn.reset!
+              checkin conn
+            else
               remove conn
             end
           end
@@ -415,20 +420,15 @@ module ActiveRecord
         elsif @connections.size < @size
           checkout_new_connection
         else
+          reap
           @available.poll(@checkout_timeout)
         end
       end
 
-      def release(conn)
-        thread_id = if @reserved_connections[current_connection_id] == conn
-          current_connection_id
-        else
-          @reserved_connections.keys.find { |k|
-            @reserved_connections[k] == conn
-          }
-        end
+      def release(conn, owner)
+        thread_id = owner.object_id
 
-        @reserved_connections.delete thread_id if thread_id
+        @reserved_connections.delete thread_id
       end
 
       def new_connection