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

Flush idle database connections #31221

Merged
merged 1 commit into from Nov 26, 2017

Conversation

Projects
None yet
7 participants
@matthewd
Member

matthewd commented Nov 25, 2017

After #28742, this seems to be a plausible much simpler take on #23815.

This is actually two features:

  1. What it says on the tin: allow the connection pool to shrink (down to zero, at the moment -- do we want to default to keeping one?) when it's not seeing enough activity to justify keeping those it has.
  2. The on_load hook, which ensures a Puma master process doesn't keep a connection open just because it needed one during boot. (Slightly related to #31173, but separate: that one's about being safer if we do keep the connection; this is about not keeping the connection at all if we don't need it.)
@kaspth

kaspth approved these changes Nov 25, 2017

@albertoalmagro

This comment has been minimized.

Show comment
Hide comment
@albertoalmagro

albertoalmagro Nov 25, 2017

Contributor

Great work 👏

Contributor

albertoalmagro commented Nov 25, 2017

Great work 👏

@SamSaffron

This comment has been minimized.

Show comment
Hide comment
@SamSaffron

SamSaffron Nov 26, 2017

Contributor

Nice! appreciate it! will be happy to let go of our monkey patch

Contributor

SamSaffron commented Nov 26, 2017

Nice! appreciate it! will be happy to let go of our monkey patch

@matthewd matthewd merged commit 3ba20c7 into rails:master Nov 26, 2017

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@matthewd matthewd deleted the matthewd:flush-idle-connections branch Nov 26, 2017

@jrafanie

This comment has been minimized.

Show comment
Hide comment
@jrafanie

jrafanie Nov 27, 2017

Contributor

Awesome @matthewd!

Contributor

jrafanie commented Nov 27, 2017

Awesome @matthewd!

@jjb

This comment has been minimized.

Show comment
Hide comment
@jjb

jjb Feb 4, 2018

Contributor

@matthewd question:

There is this behavior:

Ideally the application doesn't connect to the database during boot,
but sometimes it does. In case it did, we want to empty out the
connection pools so that a non-database-using process (e.g. a master
process in a forking server model) doesn't retain a needless
connection. If it was needed, the incremental cost of reestablishing
this connection is trivial: the rest of the pool would need to be
populated anyway.

And this behavior:

Connections currently checked out, or that were
checked in less than +minimum_idle+ seconds ago, are unaffected.

So in the "in case it did" scenario, if the puma (say) process forks are done sooner than 60 seconds after a connection was used, will the pre-fork process fail to let go of all connections before forking?

Contributor

jjb commented Feb 4, 2018

@matthewd question:

There is this behavior:

Ideally the application doesn't connect to the database during boot,
but sometimes it does. In case it did, we want to empty out the
connection pools so that a non-database-using process (e.g. a master
process in a forking server model) doesn't retain a needless
connection. If it was needed, the incremental cost of reestablishing
this connection is trivial: the rest of the pool would need to be
populated anyway.

And this behavior:

Connections currently checked out, or that were
checked in less than +minimum_idle+ seconds ago, are unaffected.

So in the "in case it did" scenario, if the puma (say) process forks are done sooner than 60 seconds after a connection was used, will the pre-fork process fail to let go of all connections before forking?

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Feb 5, 2018

Member

@jjb no; that calls flush_idle_connections! -> flush!, which will:

Disconnect all currently idle connections. Connections currently checked out are unaffected.

(and any accidentally-checked-out connection will first be returned to the pool by clear_active_connections!)

Member

matthewd commented Feb 5, 2018

@jjb no; that calls flush_idle_connections! -> flush!, which will:

Disconnect all currently idle connections. Connections currently checked out are unaffected.

(and any accidentally-checked-out connection will first be returned to the pool by clear_active_connections!)

@jjb

This comment has been minimized.

Show comment
Hide comment
@jjb

jjb Feb 5, 2018

Contributor

Thanks.

I've confirmed that 5.2.0.rc1 makes one instance of my connection management code unnecessary. Interestingly there was another instance, described in the spawning processes example, where I couldn't reproduce a problem in any version of rails. I'm not sure if I ever experienced it there or if I was just adding the management to be on the safe side.

The place where I did require the management, and now in 5.2 do not, was inside of an AR model (in a class method).

So maybe there's something different about forking in a standalone script (run with rails runner) vs inside of an AR runner (running not in a web server, but in a process forked from said standalone script).

shrug emoticon.

Contributor

jjb commented Feb 5, 2018

Thanks.

I've confirmed that 5.2.0.rc1 makes one instance of my connection management code unnecessary. Interestingly there was another instance, described in the spawning processes example, where I couldn't reproduce a problem in any version of rails. I'm not sure if I ever experienced it there or if I was just adding the management to be on the safe side.

The place where I did require the management, and now in 5.2 do not, was inside of an AR model (in a class method).

So maybe there's something different about forking in a standalone script (run with rails runner) vs inside of an AR runner (running not in a web server, but in a process forked from said standalone script).

shrug emoticon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment