From 76ce08a493fbd9f8c38d719f4c329bc2153204fc Mon Sep 17 00:00:00 2001 From: Matthew Draper Date: Wed, 6 Jul 2016 08:28:06 +0930 Subject: [PATCH 1/4] Check connection ownership before allowing a thread to release it A thread can only release a connection if it owns it, or it's owned by a thread that has died. --- .../connection_adapters/abstract_adapter.rb | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index d4b9e301bc..5140238571 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -184,7 +184,17 @@ module ActiveRecord # this method must only be called while holding connection pool's mutex def expire - @owner = nil + if in_use? + if @owner != Thread.current && @owner.alive? + 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 def unprepared_statement From f4159506d68d7d7781e1534bebbf83c0c3423562 Mon Sep 17 00:00:00 2001 From: Matthew Draper Date: Wed, 6 Jul 2016 08:37:26 +0930 Subject: [PATCH 2/4] Re-check that the connection is still stale before we reap it A concurrent thread may have also detected it to be stale, and already released (or even reassigned) it by now. Fixes #25585 --- activerecord/CHANGELOG.md | 7 +++++++ .../connection_adapters/abstract/connection_pool.rb | 2 ++ 2 files changed, 9 insertions(+) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 8521d3b07b..4637847987 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,10 @@ +* Ensure concurrent invocations of the connection reaper cannot allocate the + same connection to two threads. + + Fixes #25585. + + *Matthew Draper* + * Inspecting an object with an associated array of over 10 elements no longer truncates the array, preventing `inspect` from looping infinitely in some cases. 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 c341773be1..51bbc02b0c 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb @@ -561,6 +561,8 @@ module ActiveRecord stale_connections.each do |conn| synchronize do + next unless conn.in_use? && !conn.owner.alive? + if conn.active? conn.reset! checkin conn From f397b385c402c95b2066005b403e794fc5542868 Mon Sep 17 00:00:00 2001 From: Matthew Draper Date: Wed, 6 Jul 2016 08:45:54 +0930 Subject: [PATCH 3/4] Reduce locking by taking ownership of stale connections This way, we aren't racing other threads, so we don't need to re-check the conditional. And we no longer need to hold the lock while calling remove (which can choose to make a new connection while we wait). --- .../abstract/connection_pool.rb | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 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 51bbc02b0c..98cf97910e 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb @@ -556,19 +556,18 @@ module ActiveRecord stale_connections = synchronize do @connections.select do |conn| conn.in_use? && !conn.owner.alive? + end.each do |conn| + conn.expire + conn.lease end end stale_connections.each do |conn| - synchronize do - next unless conn.in_use? && !conn.owner.alive? - - if conn.active? - conn.reset! - checkin conn - else - remove conn - end + if conn.active? + conn.reset! + checkin conn + else + remove conn end end end From 61f4b1ff8a54ccd7e70bb4f6c0b958b28bc648fc Mon Sep 17 00:00:00 2001 From: Matthew Draper Date: Wed, 6 Jul 2016 23:50:37 +0930 Subject: [PATCH 4/4] Make connection stealing more explicit --- .../abstract/connection_pool.rb | 13 +++++++++---- .../connection_adapters/abstract_adapter.rb | 15 ++++++++++++++- activerecord/test/cases/connection_pool_test.rb | 5 ++++- 3 files changed, 27 insertions(+), 6 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 98cf97910e..bf70d4dc59 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb @@ -415,7 +415,10 @@ module ActiveRecord 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 @@ module ActiveRecord 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?) @@ -557,8 +563,7 @@ module ActiveRecord @connections.select do |conn| conn.in_use? && !conn.owner.alive? end.each do |conn| - conn.expire - conn.lease + conn.steal! end end diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index 5140238571..5747e4d1ee 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -185,7 +185,7 @@ module ActiveRecord # this method must only be called while holding connection pool's mutex def expire if in_use? - if @owner != Thread.current && @owner.alive? + if @owner != Thread.current raise ActiveRecordError, "Cannot expire connection, " << "it is owned by a different thread: #{@owner}. " << "Current thread: #{Thread.current}." @@ -197,6 +197,19 @@ module ActiveRecord 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 old_prepared_statements, @prepared_statements = @prepared_statements, false yield diff --git a/activerecord/test/cases/connection_pool_test.rb b/activerecord/test/cases/connection_pool_test.rb index a45ee281c7..09e7848bda 100644 --- a/activerecord/test/cases/connection_pool_test.rb +++ b/activerecord/test/cases/connection_pool_test.rb @@ -151,7 +151,7 @@ module ActiveRecord 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 @@ module ActiveRecord 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