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

Ensure connection reaper threads respawn in forks #36998

Merged
merged 2 commits into from Aug 20, 2019

Conversation

@jhawthorn
Copy link
Member

commented Aug 20, 2019

Adapted from #36979

This ensures that the connection pool reaper is respawned in forked processes. This is done by tracking the threads and verifying that they are alive.

This includes a previously failing test case that checks that connections are reaped within a fork.

cc @tgxworld @tenderlove @eileencodes

Allow reaper thread exit if all connections closed
Previously, once we spawned a reaper thread we would keep it around
forever. This was mostly fine because basically nothing (other than
tests) is likely to configure this time and so it should only have been
one thread.

That said, we might as well clean it up properly if all connection pools
are removed.

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

@jhawthorn jhawthorn changed the title Reaper fork Ensure connection reaper threads respawn in forks Aug 20, 2019

Ensure reaper threads respawn in forks
This broke in 3e2e8ee where it switched to one reaper thread per
process. However, the implementation will only spawn the reaping thread
if the reaping frequency has not been seen.  Since the reaping
frequencies are stored in a class instance variable, that variable is
copied when the process is forked. As such, a forked process will not
spawn a new reaping thread since the class instance variable would have
contain the reaping frequency that was seen in the parent process.

This commit tracks threads separately and checks that they both have
been spawned and are currently alive.

This adds a failing test to reaper_test.rb, based on the previous test
without forking. It also improves the test to return an failure instead
of hanging forever if the reaper is not started.

Co-authored-by: Guo Xiang Tan <tgx_world@hotmail.com>

@jhawthorn jhawthorn force-pushed the jhawthorn:reaper_fork branch from d477a97 to c72b77b Aug 20, 2019

@tenderlove
Copy link
Member

left a comment

��_��

@jhawthorn jhawthorn merged commit 7ad40b8 into rails:master Aug 20, 2019

2 checks passed

buildkite/rails Build #63191 passed (8 minutes, 37 seconds)
Details
codeclimate All good!
Details
jhawthorn added a commit that referenced this pull request Aug 20, 2019
Merge pull request #36998 from jhawthorn/reaper_fork
Ensure connection reaper threads respawn in forks
@jhawthorn

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

Backported to 6-0 as e8257e3

@tgxworld

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

Continuing the discussing from #36979 (comment)

I also didn't apply the reaper.stop/deregister_pool, I don't believe we should need it, since we can just find any GC'd pools at the next pass of the reaper.

@jhawthorn This is intentional because a pool is discarded after forking where the @connections instance variable is set to nil.

@connections = @available = @thread_cached_conns = nil

If we don't remove the reference to the parent pool from the class instance variable during discard!, the first thread that is started after forking crashes when trying to reap connections from the pool

Traceback (most recent call last):
	8: from /home/tgxworld/work/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:338:in `block in spawn_thread'
	7: from /home/tgxworld/work/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:338:in `synchronize'
	6: from /home/tgxworld/work/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:340:in `block (2 levels) in spawn_thread'
	5: from /home/tgxworld/work/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:340:in `each'
	4: from /home/tgxworld/work/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:341:in `block (3 levels) in spawn_thread'
	3: from /home/tgxworld/.asdf/installs/ruby/2.6.2/lib/ruby/2.6.0/delegate.rb:83:in `method_missing'
	2: from /home/tgxworld/work/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:650:in `reap'
	1: from /home/tgxworld/.asdf/installs/ruby/2.6.2/lib/ruby/2.6.0/monitor.rb:230:in `mon_synchronize'
/home/tgxworld/work/rails/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:651:in `block in reap': private method `select' called for nil:NilClass (NoMethodError)

Also, I feel that explicitly removing the reference of the pool in the reaper makes the intent of cleaning up references for the pool clearer.

@tgxworld

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

@jhawthorn I think we're still missing something because running the following script on latest will show the reaper thread crashing in the forked process. The thread will only be re-spawned if a connection is re-established after it has crashed which I don't think is ideal.

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'pg'
  gem 'rails', path: '/home/tgxworld/work/rails'
end

require 'active_record'

ActiveRecord::Base.establish_connection(
  :adapter => "postgresql",
  :database => "testdb",
  :reaping_frequency => 1,
)

puts "#{Process.pid} main process"

fork do
  puts "#{Process.pid} forked process"

  ActiveRecord::Base.establish_connection(
    :adapter => "postgresql",
    :database => "testdb",
    :reaping_frequency => 1,
  )

  loop { sleep 1; puts "#{Process.pid} #{Thread.list}"; }
end

loop { sleep 1; puts "#{Process.pid} #{Thread.list}" }
@jhawthorn

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

If we don't remove the reference to the parent pool from the class instance variable during discard!, the first thread that is started crashes when trying to reap connections from the pool

Ugh 😩. Good find. I don't think removing them is the best solution because there would be a (very unlikely) race condition where the reaper iteration starts before the pool is removed.

Let's tackle this in another PR. I think we should probably make reap/flush return early in that case and can test that on its own (without needing the rely on the reaper) by calling those methods directly.

@tgxworld

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

I might be wrong but I think from memory that unowned pools are always discarded first before new pools are initialized.

I think we should probably make reap/flush return early in that case and can test that on its own (without needing the rely on the reaper) by calling those methods directly.

https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb#L529-L530

Based on the above which @matthewd can confirm, I think the intent is that a discarded pool should not be interacted with.

Also, I'm still of the view that deregistering a pool from the reaper is a much simpler solution since it only involves deleting the pool from the hash which the reaper tracks. With the weakref approach, we have to check on every loop for weakref_alive? and also rescue from rescue WeakRef::RefError.

@ignatiusreza

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

I have just one question: can somebody translate Aaron Patterson's comment for me?

多分動く -> Maybe it works..

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