Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions kernel/shared/src/main/scala/cats/effect/kernel/Resource.scala
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import scala.annotation.unchecked.uncheckedVariance
import scala.concurrent.ExecutionContext
import scala.concurrent.duration.{Duration, FiniteDuration}

import java.util.concurrent.TimeoutException

/**
* `Resource` is a data structure which encodes the idea of executing an action which has an
* associated finalizer that needs to be run when the action completes.
Expand Down Expand Up @@ -1496,6 +1498,28 @@ private[effect] trait ResourceTemporal[F[_]]

def sleep(time: FiniteDuration): Resource[F, Unit] =
Resource.sleep(time)

// Overridden because the GenTemporal default is implemented in terms of
// racePair, whose Resource instance (via start) does not guarantee the
// source's finalizers have run by the time use returns (#4489, regressed by
// #4059). Resource#race has the correct finalizer semantics (#3226), so we
// express the timeout in terms of it instead.
override protected def timeoutTo[A](
fa: Resource[F, A],
duration: FiniteDuration,
fallback: Resource[F, A]): Resource[F, A] =
fa.race(Resource.sleep[F](duration)).flatMap {
case Left(a) => Resource.pure[F, A](a)
case Right(()) => fallback
}

override protected def timeout[A](fa: Resource[F, A], duration: FiniteDuration)(
implicit ev: TimeoutException <:< Throwable): Resource[F, A] =
fa.race(Resource.sleep[F](duration)).flatMap {
case Left(a) => Resource.pure[F, A](a)
case Right(()) =>
Resource.eval(F.raiseError[A](new TimeoutException(duration.toString())))
}
}

abstract private[effect] class ResourceAsync[F[_]]
Expand Down
30 changes: 30 additions & 0 deletions tests/shared/src/test/scala/cats/effect/ResourceSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1209,6 +1209,36 @@ class ResourceSuite extends BaseScalaCheckSuite with DisciplineSuite {
}
}

real("run finalisers when winning a timeout race") {
IO.ref(false).flatMap { ref =>
val res = Resource.make(ref.set(true))(_ => ref.set(false))
val timedRes = res.timeout(1.hour)
val check = timedRes.use_ *>
ref.get.ifM(IO.raiseError(new Exception("not released")), IO.unit)
check.replicateA_(10000)
}
}

real("run finalisers when losing a timeout race") {
IO.ref(true).flatMap { ref =>
val res = Resource.make(IO.sleep(100.millis))(_ => ref.set(false))
val timedRes = res.timeout(1.milli)
val check = timedRes.use_.attempt *> IO.sleep(150.millis) *>
ref.get.ifM(IO.raiseError(new Exception("not released")), IO.unit)
check.parReplicateA_(10000)
}
}

real("run nested finalisers when succeeding at the same time as the timeout") {
val go = IO.ref(false).flatMap { ref =>
val inner = Resource.make(ref.set(true))(_ => ref.set(false))
val res = Resource.make(IO.sleep(99.millis).flatMap(_ => inner.use_))(_ => IO.unit)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part is strange to me. Specifically use_ing inner in the acquire part of res. Don't get me wrong, I'd expect this test to pass (and it doesn't). But the test itself is unclear to me. Was @armanbilge's minimization here not enough to reproduce the issue?

@fommil fommil Jun 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

His minimisation is one of the three tests I added. (The other) Daniel said he expected my change to cause the value to be leaked when it completes at the same time as the timeout so I wrote this test and was surprised to find that it fails for both the mainline and my branch. The third test is a regular "timeout wins" test which seems fine for both mainline and my suggestion.

With this test I wanted a way to tell if the value has been leaked, which is why the inner Resource modifies then cleans up the ref. If you have another way of testing a leaked value that might be cleaner because there's a double use of Resource here that can be confusing.

res.timeout(100.millis).use_.attempt *> IO.sleep(150.millis) *>
ref.get.ifM(IO.unit, IO.raiseError(new Exception("inner finaliser not run")))
}
go.parReplicateA_(100000)
}

ticked("attempt - releases resource on error") { implicit ticker =>
assertCompleteAs(
IO.ref(0)
Expand Down
Loading