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

Conversation

@tgxworld
Copy link
Contributor

commented Aug 21, 2019

After forking, the connection handler will discard any connection pools that belongs to the parent process. However, it will continue to hold a reference to the connection pools of the parent process which is used for retrieval of the connection pool in the child process. Therefore, the weakref_alive? check in the connection pool reaper is insufficient since the connection pools of the parent process is never GCed.

cc @jhawthorn @eileencodes @tenderlove

@rails-bot rails-bot bot added the activerecord label Aug 21, 2019

@tgxworld tgxworld force-pushed the tgxworld:fix_connection_pool_repaer branch from 4530dae to 8f8003a Aug 21, 2019

@jhawthorn
Copy link
Member

left a comment

This is great, but I don't think we should expose pools (see comments)

@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

@@ -319,6 +319,8 @@ def initialize(pool, frequency)
@threads = {}

class << self
attr_reader :pools

This comment has been minimized.

Copy link
@jhawthorn

jhawthorn Aug 21, 2019

Member

I'd prefer we avoided exposing this attr_reader.

frequency = 0.001
@handler.establish_connection(@config.merge(reaping_frequency: frequency))

assert_equal 1, ActiveRecord::ConnectionAdapters::ConnectionPool::Reaper.pools[frequency].size

This comment has been minimized.

Copy link
@jhawthorn

jhawthorn Aug 21, 2019

Member

(similar to suggestion above)

I feel like this is testing the implementation rather than behaviour. I don't think there's a behaviour changes from this PR (please correct me if I'm wrong 😅), we're just avoiding some cycles (and objects?) by cleaning up better, so I don't think we need a test.

This comment has been minimized.

Copy link
@tgxworld

tgxworld Aug 21, 2019

Author Contributor

yup this PR ensures that we avoid looping through discarded connection pools unnecessary in the reaper so there isn’t a behaviour change. However, this might be a problem in multi-tenant Rails apps where hundreds of connection pools are used so I think we should clean up properly in the reaper. In Rails 5.2, a reaper isn’t started for discarded pools but it was using one thread per pool which was a much bigger problem which you’ve since fixed in 6.0 :)

This comment has been minimized.

Copy link
@tgxworld

tgxworld Aug 22, 2019

Author Contributor

@jhawthorn I re-read this and was wondering why you think testing implementation in this case might not be worth it. I'm not sure if we can actually test this at the acceptance test level since this involves a class instance variable that is copied due to forking while the test is trying to ensure that the class instance variable is properly cleaned up after a fork.

This comment has been minimized.

Copy link
@tgxworld

tgxworld Aug 22, 2019

Author Contributor

Actually I think I would fall back to my approach of de-registering the pool from the reaper during discard! which happens after a fork. I've confirmed that discard! always happen before the reaper is started after a fork. De-registering it ensures that we don't spend any iterations on discarded pools in the reaper which is much more similar to the behavior in 5.2.

@tgxworld tgxworld force-pushed the tgxworld:fix_connection_pool_repaer branch from 8f8003a to 2ebd650 Aug 24, 2019

@tgxworld

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2019

@jhawthorn I've updated the test case so that we don't need to rely on attr_reader :pools.

Fix `ConnectionPool::Reaper` reaping parent connection pools after fork.
After forking, the connection handler will discard any connection pools that belongs to the parent process. However, it will continue to hold a reference to the connection pools of the parent process which is used for retrieval of the connection pool in the child process. Therefore, the `weakref_alive?` check in the connection pool reaper is insufficient since the connection pools of the parent process is never GCed.

@tgxworld tgxworld force-pushed the tgxworld:fix_connection_pool_repaer branch from 2ebd650 to 97fe599 Aug 24, 2019

@@ -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.

@jhawthorn
Copy link
Member

left a comment

Looks great! Thank you

@jhawthorn jhawthorn merged commit 4a2746d into rails:master Aug 24, 2019

2 checks passed

buildkite/rails Build #63284 passed (9 minutes, 3 seconds)
Details
codeclimate All good!
Details

@tgxworld tgxworld deleted the tgxworld:fix_connection_pool_repaer branch Aug 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.