Allow restarting pool #140
Conversation
|
But why? |
|
What's wrong with |
|
It requires updating every reference to the Pool held everywhere, which in some cases is cumbersome. |
|
Ok, then I think I have a problem with the naming. Once something is shutdown, it should not be reused. Perhaps we need a |
“Restart” seems the obvious name for first shutting-down, then “starting” to put back into a usable state (which is what this method does). So while I disagree that “once shutdown it should not be reused”, I’m not here to bikeshed and so will defer to you on naming semantics.
Similar to the referenced previous issue: all connections in the pool need to all be recreated after forking. |
|
Got it, thanks for the context, now I understand what we are trying to achieve. I think I prefer |
|
Sure, just in TimedStack or also in ConnectionPool? |
|
I think the public API should be more explicit: |
Also reword those of TimedStack#shutdown for parity.
|
See updated commits. |
|
Checking in to see if there’s any further feedback as it’s been a bit, thanks. |
|
Out of curiosity, what type of connections are you pooling? I ask because many Ruby connection libraries automatically recycle the connection if they detect a PID change (i.e. after a fork). |
lib/connection_pool/timed_stack.rb
Outdated
| @@ -93,6 +95,7 @@ def shutdown(&block) | |||
|
|
|||
| shutdown_connections | |||
| end | |||
| @shutdown_block = nil if reload | |||
There was a problem hiding this comment.
Should this line go inside the mutex sync block? Otherwise there appears to be a race with pop.
There was a problem hiding this comment.
You’re correct, if pop L70 is called in another thread between L96 & L98 then pop will incorrectly raise PoolShuttingDownError. I’ve amended the commit to move this inside the synchronize.
|
In this particular case we’re using the redis gem. It does detect PID-change, but raises (src). |
|
Please update the changelog and readme. |
The implementation of shutdown from mperham#27 does not actually provide for a way for the pool to re-create connections, only render the pool unusable. This implements such a behavior. A new method is added so as to not change the existing behavior of `shutdown`.
|
Updated. |
|
Well done, thanks for your hard work and patience! |
|
This method is highly appreciated. Thanks for working on it! |
Ref: mperham/connection_pool#140 When forking a process, you need to close existing connection to avoid sharing them across processes. `shutdown` does that, but it also mark the pool as no longer usable. `connection_pool` `2.2.4` introduced `#reload` that discard existing connections, but let the pool be used again later. It's a much better fit for an `after_fork` callback.
Ref: mperham/connection_pool#140 When forking a process, you need to close existing connection to avoid sharing them across processes. `shutdown` does that, but it also mark the pool as no longer usable. `connection_pool` `2.2.4` introduced `#reload` that discard existing connections, but let the pool be used again later. It's a much better fit for an `after_fork` callback.
Ref: mperham/connection_pool#140 When forking a process, you need to close existing connection to avoid sharing them across processes. `shutdown` does that, but it also mark the pool as no longer usable. `connection_pool` `2.2.4` introduced `#reload` that discard existing connections, but let the pool be used again later. It's a much better fit for an `after_fork` callback.
The implementation of shutdown from #27 does not actually provide for a way for the pool to re-create connections, only render the pool unusable. This implements such a behavior. A new method is added so as to not change the existing behavior of
shutdown.