search

mperham / connection_pool

Generic connection pooling for Ruby
MIT License
1.63k stars 143 forks source link

Allow restarting pool #140

Closed amarshall closed 3 years ago

amarshall commented 3 years ago

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.

mperham commented 3 years ago

But why?

mperham commented 3 years ago

What's wrong with ConnectionPool.new...?

amarshall commented 3 years ago

It requires updating every reference to the Pool held everywhere, which in some cases is cumbersome.

mperham commented 3 years ago

Ok, then I think I have a problem with the naming. Once something is shutdown, it should not be reused. Perhaps we need a reload method which is used instead of shutdown? Can you explain the higher-level use case you are trying to solve? Reloading connections when DNS changes, or similar?

amarshall commented 3 years ago

Once something is shutdown, it should not be reused. Perhaps we need a reload method which is used instead of shutdown?

“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.

Can you explain the higher-level use case you are trying to solve?

Similar to the referenced previous issue: all connections in the pool need to all be recreated after forking.

mperham commented 3 years ago

Got it, thanks for the context, now I understand what we are trying to achieve. I think I prefer reload. Can you implement it without adding a new method to TimedStack, i.e. shutdown(reload=false, &block)?

amarshall commented 3 years ago

Sure, just in TimedStack or also in ConnectionPool?

mperham commented 3 years ago

I think the public API should be more explicit: ConnectionPool#reload with RDoc that explains the use case.

amarshall commented 3 years ago

See updated commits.

amarshall commented 3 years ago

Checking in to see if there’s any further feedback as it’s been a bit, thanks.

mperham commented 3 years ago

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).

amarshall commented 3 years ago

In this particular case we’re using the redis gem. It does detect PID-change, but raises (src).

mperham commented 3 years ago

Please update the changelog and readme.

amarshall commented 3 years ago

Updated.

mperham commented 3 years ago

Well done, thanks for your hard work and patience!

jbielick commented 3 years ago

This method is highly appreciated. Thanks for working on it!