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

disconnect from connection pool before forking still relevant? #1001

Closed
jjb opened this issue Jun 16, 2016 · 5 comments
Closed

disconnect from connection pool before forking still relevant? #1001

jjb opened this issue Jun 16, 2016 · 5 comments

Comments

@jjb
Copy link
Contributor

@jjb jjb commented Jun 16, 2016

I found this buried in the readme:

If you're preloading your application and using ActiveRecord, it's recommended that you close any connections to the database here to prevent connection leakage:

# config/puma.rb
before_fork do
  ActiveRecord::Base.connection_pool.disconnect!
end

I've never seen this recommendation mentioned in any discussions elsewhere, and the configuration is not included in puma's own heroku plugin https://github.com/puma/puma-heroku/blob/master/lib/puma/plugin/heroku.rb

Is the recommendation still relevant? If so, should it go into the heroku plugin? (I can do a PR)

@evanphx
Copy link
Member

@evanphx evanphx commented Jun 20, 2016

@jjb It can't hurt to go into the official heroku plugin. @schneems thoughts?

@schneems
Copy link
Contributor

@schneems schneems commented Jun 20, 2016

Interesting. I guess disconnecting before forking has the same effect as manually connecting after the fork. I prefer that method otherwise if there's a weird race condition where you've done a query in a separate thread or something you might accidentally create another connection maybe?

I don't see adding it as a bad thing, however I also don't entirely think it's necessary. The establish_connection should fix any connection pool problems.

Actually, re-read that comment and I think we should be doing it. It's about connection leaks. Right now you might be left with X connections to the database open that you'll never use for each dyno. Not a big deal for a few dynos but if you've got a lot of them connections in PG aren't free. We should probably add it. Can you send a patch to put it in the plugin @jjb ?

@jjb
Copy link
Contributor Author

@jjb jjb commented Jun 20, 2016

done!

@jjb jjb closed this Jun 21, 2016
christiannelson added a commit to carbonfive/raygun-rails that referenced this issue Sep 20, 2016
See puma/puma#1001 and rails/rails#26314 for background.
@kjvarga
Copy link

@kjvarga kjvarga commented Jan 18, 2019

Is this still needed under Rails 5.2? It looks like Rails 5.2 automatically closes connections that were held in other processes.

@jjb
Copy link
Contributor Author

@jjb jjb commented Jan 18, 2019

See discussion about rails 5.2 here, in my large unmerged 😭 rails guide rails/rails#29807

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants