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 connections reaper not running after forking. #36979

Conversation

@tgxworld
Copy link
Contributor

commented Aug 19, 2019

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.

Also reverts the use of WeakRef introduced in
6302e56 since we properly deregisters
connection pools from the reaper upon discard/disconnect.

Script to reproduce the problem

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}" }

cc @jhawthorn @rafaelfranca @tenderlove

@rails-bot rails-bot bot added the activerecord label Aug 19, 2019
@tgxworld tgxworld force-pushed the tgxworld:fix_reaper_not_running_in_forked_process branch 6 times, most recently from 4a13db8 to 969cf0c Aug 19, 2019
@eileencodes eileencodes added this to the 6.0.1 milestone Aug 19, 2019
@rafaelfranca rafaelfranca requested a review from jhawthorn Aug 19, 2019
@jhawthorn

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

Thanks for catching and investigating.

I believe a weakref is still necessary, otherwise connection pools can never be garbage collected. For example this will count up forever never freeing a pool

loop do
  ActiveRecord::Base.establish_connection(
    :adapter => "mysql2",
    :database => "test",
    :reaping_frequency => 2,
  )

  GC.start
  p ObjectSpace.each_object(ActiveRecord::ConnectionAdapters::ConnectionPool).count
  sleep 0.1
end
@tgxworld tgxworld force-pushed the tgxworld:fix_reaper_not_running_in_forked_process branch from 969cf0c to a283293 Aug 20, 2019
@tgxworld

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

@jhawthorn Thank you for the background regarding the use of weakref 👍 I've updated the PR to properly remove the connection pool from the reaper upon disconnect as well.

@tgxworld tgxworld force-pushed the tgxworld:fix_reaper_not_running_in_forked_process branch from a283293 to 9836160 Aug 20, 2019
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.

Also reverts the use of `WeakRef` introduced in
6302e56 since we properly deregisters
connection pools from the reaper upon discard/disconnect.
@tgxworld tgxworld force-pushed the tgxworld:fix_reaper_not_running_in_forked_process branch from 9836160 to 9837242 Aug 20, 2019
@jhawthorn

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

@tgxworld Could you take a look at #36998 based on this ❤️

I've tried to test the behaviour we expect to see (connections in a fork are reaped) rather than the implementation specifics. I'm also not comfortable using ObjectSpace in a test, I think it will be unreliable even if we explicitly GC.

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.

@tgxworld

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

Closed by #36998

@tgxworld tgxworld closed this Aug 20, 2019
@tgxworld tgxworld deleted the tgxworld:fix_reaper_not_running_in_forked_process branch Aug 20, 2019
@tgxworld

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

I've tried to test the behaviour we expect to see (connections in a fork are reaped) rather than the implementation specifics.

@jhawthorn Thank you for this 👍 I read through your test cases and I agree that testing the behavior is much more resilient than testing for the implementation details.

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