fix(pool): returning closed resources to the pool #154
Conversation
|
I agree, but in your context, how is the connection closed in the first place? I would like to have a spec to cover that scenario. It seems easy to reintroduce in future refactors otherwise. |
|
We did this: place-labs/crystal-pg@5db17e3 it might actually be worthwhile calling close in the |
|
Spec added :) |
| @@ -141,7 +141,9 @@ module DB | |||
| idle_pushed = false | |||
|
|
|||
| sync do | |||
| if can_increase_idle_pool | |||
| if resource.responds_to?(:closed?) && resource.closed? | |||
There was a problem hiding this comment.
Is this condition necessary for any practical application? I think it would be better to require a #closed? method. The interface already expects #close, so it would be nearby.
This might be a breaking change, but I'd accept that in order to avoid hitting this problem when a resource class doesn't implement closed?.
There was a problem hiding this comment.
I would like to say that is not needed, but some people want to use the pool with http client. The HTTP::Client has a #close, but has no #closed?.
There was a problem hiding this comment.
We should probably add that to HTTP::Client 🤔
There was a problem hiding this comment.
We could, but the HTTP::Client might be reopened. I'm not sure what a good choice for interacting with it through a pool would be. But for now the responds_to? needs to stay because of that.
There was a problem hiding this comment.
I was wondering if there's an option for more conservative behaviour when the resource does not respond to #closed?. But I don't think there is one except unconditionally deletion, which would defeat the purpose of a pool.
Maybe it makes sense to consider a resource type that does not implement #closed? as being able to reset/reopen itself on subsequent checkouts.
There was a problem hiding this comment.
Maybe it makes sense to consider a resource type that does not implement #closed? as being able to reset/reopen itself on subsequent checkouts.
And that would be the current behavior. Or you think it's not?
There was a problem hiding this comment.
I think this is good to go.
Regarding
it might actually be worthwhile calling close in the DB::ConnectionLost initialiser too
Yes, I think that is a good place to put it. That will also solve a leak in the prepared statement cache where the connection are cleaned if they are closed.
I was thinking if ConnectionRefused should also be closed, but in that exception type that is used upon connection creation there is no connection object. Because it couldn't initialize. So I think is on each driver to clean up resources in this situation.
We can add/discuss the connection close upon DB::ConnectionLost creation on a following PR.
|
Thanks @stakach! |
returning closed resources is undesirable