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

Automatically drop all connections after fork #166

Merged
merged 2 commits into from
Oct 4, 2022

Conversation

casperisfine
Copy link
Contributor

Fix: #165

Using connections inherited from a parent process can have very nasty consequences and it's pretty much never desired.

This patch use the Process._fork API added in Ruby 3.1 to automatically detect when a fork is happening, and when it does, drop all existing connections.

Notes

Performance

ObjectSpace.each_object is very slow for large applications:

[1] pry(main)> Benchmark.realtime { ObjectSpace.each_object(ConnectionPool) { |c| c }.size }
=> 0.06875399989075959
[2] pry(main)> Rails.application.eager_load!
=> nil
[3] pry(main)> Benchmark.realtime { ObjectSpace.each_object(ConnectionPool) { |c| c }.size }
=> 0.14990400010719895
[4] pry(main)> Benchmark.realtime { fork { exit! }}
=> 0.004583500092849135

So I'd really recommend to keep track of instance with ObjectSpace::WeakMap. I'll push a commit in another branch and let you decide.

Connection closing

Since ConnectionPool doesn't know how to close a connection, this feature simply drop the references, and assume the Ruby GC will do the right thing.

@casperisfine
Copy link
Contributor Author

Here's what keeping track of instances in a WeakMap would involve: casperisfine@3ff19dd.

# We're on after fork, so we know all other threads are dead.
# All we need to do is to ensure the main thread doesn't have a
# checked out connection
pool.checkin(force: true)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this code running in the child main thread? Any main thread connection would have been checked out by the parent process's main thread. Will that use the same thread locals?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will that use the same thread locals?

yes, the main thread state is kept.

So this codepath is to handle code such as:

pool.checkout
fork

or

pool.with do
  fork
end

Fix: mperham#165

Using connections inherited from a parent process can have
very nasty consequences and it's pretty much never desired.

This patch use the `Process._fork` API added in Ruby 3.1 to
automatically detect when a fork is happening, and when it
does, drop all existing connections.
@casperisfine casperisfine force-pushed the auto-reload-on-fork branch 3 times, most recently from 64c09b7 to e197083 Compare September 27, 2022 16:08
Copy link

@eregon eregon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

This is much faster than relying on `ObjectSpace.each_object`
@mperham mperham merged commit 428c06f into mperham:main Oct 4, 2022
@casperisfine
Copy link
Contributor Author

Thanks! ❤️

@hugobarthelemy
Copy link

A new release is planned to integrate this PR?

@mperham
Copy link
Owner

mperham commented Dec 6, 2022

Eventually. Do you need it soon for some reason?

@hugobarthelemy
Copy link

Periodically, nearly restart or scaling event, we have more connections open than the theoretical count. We don't investigate a lot because we have needed to upscale Redis plan for other reasons and pics it's never more than 2x higher than the theoretical count. But we would appreciate updating our gem with this feature only to see if it resolves our problem.

@mperham
Copy link
Owner

mperham commented Dec 6, 2022

You could run main, right?

@hugobarthelemy
Copy link

We could. It's not very clean, but we could.

@mperham
Copy link
Owner

mperham commented Dec 6, 2022

I could use the feedback, let me know if it helps. Not sure what you mean by clean, it’s one line in your Gemfile.

@oskarpearson
Copy link

I could use the feedback, let me know if it helps.

We've been running 428c06f for a couple months now without any issues on a system that's probably processed 40 million jobs over the time.

@mperham
Copy link
Owner

mperham commented Mar 24, 2023

2.4.0 is out with this new feature.

casperisfine pushed a commit to casperisfine/net-http-persistent that referenced this pull request Mar 25, 2023
Fix: mperham/connection_pool#172
Ref: mperham/connection_pool#166

ConnectionPool descendants must accept `checking(force: true)`.

When it's called, all the cached connection should be checked back in,
and the `Thread.current` state reset.

Additionally, making `Connection` respond to `close` allow them
to be automatically closed on fork.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatic fork detection
6 participants