Merge pull request rails#25707 from matthewd/double-reap

Don't reap connections that have already been reassigned

Author: matthewd

activerecord/CHANGELOG.md

@@ -1,3 +1,11 @@
+*   Ensure concurrent invocations of the connection reaper cannot allocate the
+    same connection to two threads.
+
+    Fixes #25585.
+
+    *Matthew Draper*
+
+
 ## Rails 5.0.0 (June 30, 2016) ##
 
 *   Inspecting an object with an associated array of over 10 elements no longer

activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb

@@ -415,7 +415,10 @@ def disconnect(raise_on_acquisition_timeout = true)
         with_exclusively_acquired_all_connections(raise_on_acquisition_timeout) do
           synchronize do
             @connections.each do |conn|
-              checkin conn
+              if conn.in_use?
+                conn.steal!
+                checkin conn
+              end
               conn.disconnect!
             end
             @connections = []
@@ -447,7 +450,10 @@ def clear_reloadable_connections(raise_on_acquisition_timeout = true)
         with_exclusively_acquired_all_connections(raise_on_acquisition_timeout) do
           synchronize do
             @connections.each do |conn|
-              checkin conn
+              if conn.in_use?
+                conn.steal!
+                checkin conn
+              end
               conn.disconnect! if conn.requires_reloading?
             end
             @connections.delete_if(&:requires_reloading?)
@@ -556,17 +562,17 @@ def reap
         stale_connections = synchronize do
           @connections.select do |conn|
             conn.in_use? && !conn.owner.alive?
+          end.each do |conn|
+            conn.steal!
           end
         end
 
         stale_connections.each do |conn|
-          synchronize do
-            if conn.active?
-              conn.reset!
-              checkin conn
-            else
-              remove conn
-            end
+          if conn.active?
+            conn.reset!
+            checkin conn
+          else
+            remove conn
           end
         end
       end

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

@@ -184,7 +184,30 @@ def schema_cache=(cache)
 
       # this method must only be called while holding connection pool's mutex
       def expire
-        @owner = nil
+        if in_use?
+          if @owner != Thread.current
+            raise ActiveRecordError, "Cannot expire connection, " <<
+              "it is owned by a different thread: #{@owner}. " <<
+              "Current thread: #{Thread.current}."
+          end
+
+          @owner = nil
+        else
+          raise ActiveRecordError, 'Cannot expire connection, it is not currently leased.'
+        end
+      end
+
+      # this method must only be called while holding connection pool's mutex (and a desire for segfaults)
+      def steal! # :nodoc:
+        if in_use?
+          if @owner != Thread.current
+            pool.send :remove_connection_from_thread_cache, self, @owner
+
+            @owner = Thread.current
+          end
+        else
+          raise ActiveRecordError, 'Cannot steal connection, it is not currently leased.'
+        end
       end
 
       def unprepared_statement

activerecord/test/cases/connection_pool_test.rb

@@ -151,7 +151,7 @@ def test_reap_inactive
 
         assert_equal 1, active_connections(@pool).size
       ensure
-        @pool.connections.each(&:close)
+        @pool.connections.each { |conn| conn.close if conn.in_use? }
       end
 
       def test_remove_connection
@@ -432,6 +432,9 @@ def test_bang_versions_of_disconnect_and_clear_reloadable_connections_if_unable_
             Thread.new { @pool.send(group_action_method) }.join
             # assert connection has been forcefully taken away from us
             assert_not @pool.active_connection?
+
+            # make a new connection for with_connection to clean up
+            @pool.connection
           end
         end
       end