Track owning thread for connection pool #14360
Conversation
|
Nice. Looks good. |
|
Agree. |
|
@matthewd can you rebase this and I'll merge it in? |
It wasn't doing anything beyond clearing the statement cache.
.. not a general timeout. Now, if a thread checks out a connection then dies, we can immediately recover that connection and re-use it. This should alleviate the pool exhaustion discussed in rails#12867. More importantly, it entirely avoids the potential issues of the reaper attempting to check whether connections are still active: as long as the owning thread is alive, the connection is its business alone. As a no-op reap is now trivial (only entails checking a thread status per connection), we can also perform one in-line any time we decide to sleep for a connection.
Track owning thread for connection pool
|
will this be merged into 4-0-stable? we are having major issues with exhaustion issues on Heroku with PG when a connection gets interrupted in Sidekiq and never gets reaped. This wasn't nearly as much of a problem in 3.2.13-17, so this has proven to be a fairly crippling regression for us. Thanks for holistically attacking this issue in this pull request and several others! |
|
No it will not. Although it is a bug fix it may introduce a regression in a stable version. We prefer to keep it only at 4.1.x |
|
Fair enough, for I don't see it in 4-1-stable branch either. Any ETA on when it will be incorporated there? |
|
Oh, my mistake I thought it was there. Since it is not the same rule applies for 4.1.x, we will see it only on 4.2 |
|
@matthewd @tenderlove this totally breaks us, we need to be able to track last_use otherwise we have no way to drain the pool on inactivity. see: https://github.com/discourse/discourse/blob/master/lib/freedom_patches/pool_drainer.rb |
The reaper was introduced back in cde7692 The goal is to detect checked out connections owned by dead threads, and make them available again. To do this, a thread check connections every 60 seconds, which IMO isn't nearly enough. But more importabtly since rails#14360 the reaper isn't really useful because in the checkout path, if we can't immediately checkout a connection, we execute the reaping routine, so IMO it's not really justified to have a background thread for this.
Now, if a thread checks out a connection then dies, we can immediately recover that connection and re-use it.
This should alleviate the pool exhaustion discussed in #12867. More importantly, it entirely avoids the potential issues of the reaper attempting to check whether connections are still active: as long as the owning thread is alive, the connection is its business alone.
/cc @tenderlove @jeremy
I kept the #reset! fix in a separate commit... it seems unrelated, apart from having been spotted while dealing with this. Let me know if it should be squashed anyway.