Make ActiveRecord ConnectionPool.connections thread-safe. (#36473)

* Make ActiveRecord `ConnectionPool.connections` thread-safe.

ConnectionPool documentation is clear on the need to synchronize
access to @connections but also states that public methods do not
require synchronization. Existing code exposed @connections
directly via attr_reader. The fix uses synchronize() to lock
@connections then returns a copy to the caller using Array.dup().

Includes comments on the connections method that thread-safe access
to the connections array does not imply thread-safety of accessing
methods on the actual connections.

Adds a test-case that modifies the pool using a supported method
in one thread  while a second thread accesses pool.connections.
The test fails without this patch.

Fixes #36465.

* Update activerecord/test/cases/connection_pool_test.rb

[jeffdoering + Rafael Mendonça França]

activerecord/CHANGELOG.md

@@ -1,3 +1,9 @@
+*   Make ActiveRecord `ConnectionPool.connections` method thread-safe.
+
+    Fixes #36465.
+
+    *Jeff Doering*
+
 *   Add support for multiple databases to `rails db:abort_if_pending_migrations`.
 
     *Mark Lee*

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

@@ -357,7 +357,7 @@ module ActiveRecord
       include ConnectionAdapters::AbstractPool
 
       attr_accessor :automatic_reconnect, :checkout_timeout, :schema_cache
-      attr_reader :spec, :connections, :size, :reaper
+      attr_reader :spec, :size, :reaper
 
       # Creates a new ConnectionPool object. +spec+ is a ConnectionSpecification
       # object which describes database connection information (e.g. adapter,
@@ -470,6 +470,21 @@ module ActiveRecord
         synchronize { @connections.any? }
       end
 
+      # Returns an array containing the connections currently in the pool.
+      # Access to the array does not require synchronization on the pool because
+      # the array is newly created and not retained by the pool.
+      #
+      # However; this method bypasses the ConnectionPool's thread-safe connection
+      # access pattern. A returned connection may be owned by another thread,
+      # unowned, or by happen-stance owned by the calling thread.
+      #
+      # Calling methods on a connection without ownership is subject to the
+      # thread-safety guarantees of the underlying method. Many of the methods
+      # on connection adapter classes are inherently multi-thread unsafe.
+      def connections
+        synchronize { @connections.dup }
+      end
+
       # Disconnects all connections in the pool, and clears the pool.
       #
       # Raises:

activerecord/test/cases/connection_pool_test.rb

@@ -694,6 +694,28 @@ module ActiveRecord
         end
       end
 
+      def test_public_connections_access_threadsafe
+        _conn1 = @pool.checkout
+        conn2 = @pool.checkout
+
+        connections = @pool.connections
+        found_conn = nil
+
+        # Without assuming too much about implementation
+        # details make sure that a concurrent change to
+        # the pool is thread-safe.
+        connections.each_index do |idx|
+          if connections[idx] == conn2
+            Thread.new do
+              @pool.remove(conn2)
+            end.join
+          end
+          found_conn = connections[idx]
+        end
+
+        assert_not_nil found_conn
+      end
+
       private
         def with_single_connection_pool
           one_conn_spec = ActiveRecord::Base.connection_pool.spec.dup