Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix `ConnectionPool::Reaper` reaping parent connection pool after fork. #37002

Merged
merged 2 commits into from Aug 24, 2019
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -336,7 +336,10 @@ def spawn_thread(frequency)
while running
sleep t
@mutex.synchronize do
@pools[frequency].select!(&:weakref_alive?)
@pools[frequency].select! do |pool|
pool.weakref_alive? && !pool.discarded?
end

@pools[frequency].each do |p|
p.reap
p.flush
@@ -532,14 +535,18 @@ def disconnect!
# See AbstractAdapter#discard!
def discard! # :nodoc:
synchronize do
return if @connections.nil? # already discarded
return if self.discarded?
@connections.each do |conn|
conn.discard!
end
@connections = @available = @thread_cached_conns = nil
end
end

def discarded? # :nodoc:
@connections.nil?
end

This comment has been minimized.

Copy link
@jhawthorn

jhawthorn Aug 21, 2019

Member

I like this a lot more than checking @connections directly


# Clears the cache which maps classes and re-connects connections that
# require reloading.
#
@@ -648,7 +655,7 @@ def remove(conn)
# or a thread dies unexpectedly.
def reap
stale_connections = synchronize do
return unless @connections
return if self.discarded?
@connections.select do |conn|
conn.in_use? && !conn.owner.alive?
end.each do |conn|
@@ -673,7 +680,7 @@ def flush(minimum_idle = @idle_timeout)
return if minimum_idle.nil?

idle_connections = synchronize do
return unless @connections
return if self.discarded?
@connections.select do |conn|
!conn.in_use? && conn.seconds_idle >= minimum_idle
end.each do |conn|
@@ -204,6 +204,62 @@ def test_connection_pools
assert_equal([@pool], @handler.connection_pools)
end

def test_a_class_using_custom_pool_and_switching_back_to_primary
klass2 = Class.new(Base) { def self.name; "klass2"; end }

assert_same klass2.connection, ActiveRecord::Base.connection

pool = klass2.establish_connection(ActiveRecord::Base.connection_pool.spec.config)
assert_same klass2.connection, pool.connection
assert_not_same klass2.connection, ActiveRecord::Base.connection

klass2.remove_connection

assert_same klass2.connection, ActiveRecord::Base.connection
end

class ApplicationRecord < ActiveRecord::Base
self.abstract_class = true
end

class MyClass < ApplicationRecord
end

def test_connection_specification_name_should_fallback_to_parent
Object.send :const_set, :ApplicationRecord, ApplicationRecord

klassA = Class.new(Base)
klassB = Class.new(klassA)
klassC = Class.new(MyClass)

assert_equal klassB.connection_specification_name, klassA.connection_specification_name
assert_equal klassC.connection_specification_name, klassA.connection_specification_name

assert_equal "primary", klassA.connection_specification_name
assert_equal "primary", klassC.connection_specification_name

klassA.connection_specification_name = "readonly"
assert_equal "readonly", klassB.connection_specification_name

ActiveRecord::Base.connection_specification_name = "readonly"
assert_equal "readonly", klassC.connection_specification_name
ensure
Object.send :remove_const, :ApplicationRecord
ActiveRecord::Base.connection_specification_name = "primary"
end

def test_remove_connection_should_not_remove_parent
klass2 = Class.new(Base) { def self.name; "klass2"; end }
klass2.remove_connection
assert_not_nil ActiveRecord::Base.connection
assert_same klass2.connection, ActiveRecord::Base.connection
end

def test_default_handlers_are_writing_and_reading
assert_equal :writing, ActiveRecord::Base.writing_role
assert_equal :reading, ActiveRecord::Base.reading_role
end

if Process.respond_to?(:fork)
def test_connection_pool_per_pid
object_id = ActiveRecord::Base.connection.object_id
@@ -352,62 +408,6 @@ def test_pool_from_any_process_for_uses_most_recent_spec
file.unlink
end
end

def test_a_class_using_custom_pool_and_switching_back_to_primary
klass2 = Class.new(Base) { def self.name; "klass2"; end }

assert_same klass2.connection, ActiveRecord::Base.connection

pool = klass2.establish_connection(ActiveRecord::Base.connection_pool.spec.config)
assert_same klass2.connection, pool.connection
assert_not_same klass2.connection, ActiveRecord::Base.connection

klass2.remove_connection

assert_same klass2.connection, ActiveRecord::Base.connection
end

class ApplicationRecord < ActiveRecord::Base
self.abstract_class = true
end

class MyClass < ApplicationRecord
end

def test_connection_specification_name_should_fallback_to_parent
Object.send :const_set, :ApplicationRecord, ApplicationRecord

klassA = Class.new(Base)
klassB = Class.new(klassA)
klassC = Class.new(MyClass)

assert_equal klassB.connection_specification_name, klassA.connection_specification_name
assert_equal klassC.connection_specification_name, klassA.connection_specification_name

assert_equal "primary", klassA.connection_specification_name
assert_equal "primary", klassC.connection_specification_name

klassA.connection_specification_name = "readonly"
assert_equal "readonly", klassB.connection_specification_name

ActiveRecord::Base.connection_specification_name = "readonly"
assert_equal "readonly", klassC.connection_specification_name
ensure
Object.send :remove_const, :ApplicationRecord
ActiveRecord::Base.connection_specification_name = "primary"
end

def test_remove_connection_should_not_remove_parent
klass2 = Class.new(Base) { def self.name; "klass2"; end }
klass2.remove_connection
assert_not_nil ActiveRecord::Base.connection
assert_same klass2.connection, ActiveRecord::Base.connection
end

def test_default_handlers_are_writing_and_reading
assert_equal :writing, ActiveRecord::Base.writing_role
assert_equal :reading, ActiveRecord::Base.reading_role
end
end
end
end
@@ -5,23 +5,13 @@
module ActiveRecord
module ConnectionAdapters
class ReaperTest < ActiveRecord::TestCase
attr_reader :pool

def setup
super
@pool = ConnectionPool.new ActiveRecord::Base.connection_pool.spec
end

teardown do
@pool.connections.each(&:close)
end

class FakePool
attr_reader :reaped
attr_reader :flushed

def initialize
def initialize(discarded: false)
@reaped = false
@discarded = discarded
end

def reap
@@ -31,6 +21,14 @@ def reap
def flush
@flushed = true
end

def discard!
@discarded = true
end

def discarded?
@discarded
end
end

# A reaper with nil time should never reap connections
@@ -40,6 +38,8 @@ def test_nil_time
reaper = ConnectionPool::Reaper.new(fp, nil)
reaper.run
assert_not fp.reaped
ensure
fp.discard!

This comment has been minimized.

Copy link
@tgxworld

tgxworld Aug 24, 2019

Author Contributor

With the new class instance variable that stores the pools for reaping, we need to clean up whenever we create a new pool to avoid leaking state in each test case.

end

def test_some_time
@@ -53,17 +53,26 @@ def test_some_time
end
assert fp.reaped
assert fp.flushed
ensure
fp.discard!
end

def test_pool_has_reaper
spec = ActiveRecord::Base.connection_pool.spec.dup
pool = ConnectionPool.new spec

assert pool.reaper
ensure
pool.discard!
end

def test_reaping_frequency_configuration
spec = ActiveRecord::Base.connection_pool.spec.dup
spec.config[:reaping_frequency] = "10.01"
pool = ConnectionPool.new spec
assert_equal 10.01, pool.reaper.frequency
ensure
pool.discard!
end

def test_connection_pool_starts_reaper
@@ -80,6 +89,8 @@ def test_connection_pool_starts_reaper

wait_for_conn_idle(conn)
assert_not_predicate conn, :in_use?
ensure
pool.discard!
end

def test_reaper_works_after_pool_discard
@@ -136,6 +147,26 @@ def test_connection_pool_starts_reaper_in_fork

Process.waitpid(pid)
assert $?.success?
ensure
pool.discard!
end

def test_reaper_does_not_reap_discarded_connection_pools
discarded_pool = FakePool.new(discarded: true)
pool = FakePool.new
frequency = 0.001

ConnectionPool::Reaper.new(discarded_pool, frequency).run
ConnectionPool::Reaper.new(pool, frequency).run

until pool.flushed
Thread.pass
end

assert_not discarded_pool.reaped
assert pool.reaped
ensure
pool.discard!
end

def new_conn_in_thread(pool)
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.