-
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
Preserve ExitCase in resources + fix memory leak #535
base: main
Are you sure you want to change the base?
Conversation
def loop: F[Unit] = for { | ||
now <- Temporal[F].monotonic | ||
_ <- { | ||
def loop: F[Unit] = Temporal[F].monotonic |
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.
thought: this change also avoids a memory leak on Scala 3 (due to better-monadic-for not eliminating the final map
).
tl;dr previously it was
def loop = for {
x <- fx
_ <- (somethingSomething >> loop)
} yield ()
which desugars to
fx.flatMap { x => (somethingSomething >> loop).map(_ => ()) }
which allocates a Map
node on every run and never cleans it up (the same goes for the top-level flatMap
, and apparently the lambdas too).
Proof: run this for a minute or so.
//> using dep "org.typelevel::cats-effect:3.5.3"
import cats.effect.*
def loop: IO[Unit] = for {
_ <- IO.unit
_ <- IO.unit >> loop
} yield ()
object CatsDemo extends IOApp.Simple {
def run = loop
}
@@ -199,25 +206,26 @@ object KeyPool { | |||
val (m_, toDestroy) = findStale(now, idleCount, m) | |||
( | |||
m_, | |||
toDestroy.traverse_(_._2._2).attempt.flatMap { | |||
// In this context, we're closing the resource due to it not being used for a while - hence a Succeeded exit case. |
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.
thought: special case here: everywhere else we have an ExitCase to propagate, but the reaper deliberately closes resources out of thin air: I figured Succeeded makes the most sense for a resource that's no longer necessary. Canceled
didn't sound right, but happy to discuss if anyone disagrees
Another stop on my "allocated is an antipattern" crusade. For previous editions, see typelevel/natchez-http4s#36 and typelevel/natchez#981.