Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

iRevive
Copy link
Contributor

@iRevive iRevive commented Nov 10, 2021

My attempt to fix #291

}
}
case PoolOpen(_, borrowed, m2) =>
borrowed.toList.traverse { case (_, (_, destroy)) => destroy.attempt.void } >>
Copy link
Contributor Author

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) =>
Copy link
Contributor Author

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.

@rossabaker rossabaker self-requested a review November 14, 2021 02:02
@ChristopherDavenport
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A borrowed resource can postpone its termination
2 participants