From 05ee662430bf362e25e60c1f7d6d54dafa7e3bba Mon Sep 17 00:00:00 2001
From: jeffdoering <jdoering@alum.mit.edu>
Date: Thu, 13 Jun 2019 11:23:13 -0700
Subject: [PATCH] Make ActiveRecord `ConnectionPool.connections` thread-safe.
 (#36473)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* 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                     |  6 +++++
 .../abstract/connection_pool.rb               | 17 +++++++++++++-
 .../test/cases/connection_pool_test.rb        | 22 +++++++++++++++++++
 3 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 06839b48b3034..a025fe37a29c4 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,9 @@
+*   Make ActiveRecord `ConnectionPool.connections` method thread-safe.
+
+    Fixes #36465.
+
+    *Jeff Doering*
+
 *   Fix sqlite3 collation parsing when using decimal columns.
 
     *Martin R. Schuster*
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 7dd5169e43ef9..4c06016a2d0fa 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb
@@ -359,7 +359,7 @@ def run
       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,
@@ -472,6 +472,21 @@ def connected?
         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:
diff --git a/activerecord/test/cases/connection_pool_test.rb b/activerecord/test/cases/connection_pool_test.rb
index 82f5f45a4c0ae..ccbb6e16cdcc6 100644
--- a/activerecord/test/cases/connection_pool_test.rb
+++ b/activerecord/test/cases/connection_pool_test.rb
@@ -694,6 +694,28 @@ def test_connection_pool_stat
         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