diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index cdad92a0cbedd3af3215c3515c7628c96cae6a92..f666d102e78f6b22b236159084fb7c6baee89917 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,9 @@ +* Make ActiveRecord `ConnectionPool.connections` method thread-safe. + + Fixes #36465. + + *Jeff Doering* + * Assign all attributes before calling `build` to ensure the child record is visible in `before_add` and `after_add` callbacks for `has_many :through` associations. 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 d054473539af0d499346789bd3382c284edad9c4..d87578e74284f728cbe5fb24c003c304308a737f 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb @@ -310,7 +310,7 @@ def run include QueryCache::ConnectionPoolConfiguration 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, @@ -423,6 +423,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 2d00cf651a54bca8ab3b9eba173f40124580fe81..9cdf62daafa267ee80a2eb3e41b89ef2359eae93 100644 --- a/activerecord/test/cases/connection_pool_test.rb +++ b/activerecord/test/cases/connection_pool_test.rb @@ -652,6 +652,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