-
Notifications
You must be signed in to change notification settings - Fork 15
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
Destroy borrowed resources during cleanup #293
base: main
Are you sure you want to change the base?
Conversation
} | ||
} | ||
case PoolOpen(_, borrowed, m2) => | ||
borrowed.toList.traverse { case (_, (_, destroy)) => destroy.attempt.void } >> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to invoke a user-specified callback in such an unusual scenario.
E.g.
class Builder[F[_]: Temporal, A, B] private[keypool] {
...
def withOnBorrowedTermination(f: List[(A, F[Unit])] => F[Unit]): Builder
}
@@ -196,7 +197,7 @@ object KeyPool { | |||
kpVar.get.map(pm => | |||
pm match { | |||
case PoolClosed() => (0, Map.empty) | |||
case PoolOpen(idleCount, m) => | |||
case PoolOpen(idleCount, _, m) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, a number of borrowed connections can be a part of the exposed state. It's a binary breaking change (if we talk about KeyPool#state
), but this data can be made available via another method.
Moreover, if represent a borrowed resource as (B, F[Unit], FiniteDuration)
, leaking resources can be spotted.
I've been working on something that takes this to a much further degree. The one major takeaway I've had is that you don't want this to be always true. It seems in line with the current implementation though. But this current implementation needs a lot more power to handle even more complex pooling scenarios. |
My attempt to fix #291