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

Implement a safer Resource.attempt which releases acquired resource(s) in case of error #4128

Open
wants to merge 12 commits into
base: series/3.x
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ private[effect] object JvmCpuStarvationMetrics {
case (mbeanServer, _) => IO.blocking(mbeanServer.unregisterMBean(mBeanObjectName))
}
.map(_._2)
.handleErrorWith[CpuStarvationMetrics, Throwable] { th =>
.handleErrorWith[CpuStarvationMetrics] { th =>
Copy link
Member Author

Choose a reason for hiding this comment

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

This change because handleErrorWith[E, A] now requires explicit ApplicativeError

Resource.eval(Console[IO].errorln(warning(th))).map(_ => new NoOpCpuStarvationMetrics)
}
}
Expand Down
80 changes: 57 additions & 23 deletions kernel/shared/src/main/scala/cats/effect/kernel/Resource.scala
Original file line number Diff line number Diff line change
Expand Up @@ -700,29 +700,52 @@ sealed abstract class Resource[F[_], +A] extends Serializable {
}
}

def attempt[E](implicit F: ApplicativeError[F, E]): Resource[F, Either[E, A]] =
this match {
case Allocate(resource) =>
Resource.applyFull { poll =>
resource(poll).attempt.map {
case Left(error) => (Left(error), (_: ExitCase) => F.unit)
case Right((a, release)) => (Right(a), release)
}
}
case Bind(source, f) =>
Resource.unit.flatMap(_ => source.attempt).flatMap {
case Left(error) => Resource.pure(error.asLeft)
case Right(s) => f(s).attempt
@deprecated("Use overload with MonadCancelThrow", "3.6.0")
def attempt[E](F: ApplicativeError[F, E]): Resource[F, Either[E, A]] =
F match {
case x: Sync[F] =>
attempt(x).asInstanceOf[Resource[F, Either[E, A]]]
case _ =>
implicit val x: ApplicativeError[F, E] = F
this match {
case Allocate(resource) =>
Resource.applyFull { poll =>
resource(poll).attempt.map {
case Left(error) => (Left(error), (_: ExitCase) => F.unit)
case Right((a, release)) => (Right(a), release)
}
}
case Bind(source, f) =>
Resource.unit.flatMap(_ => source.attempt(F)).flatMap {
case Left(error) => Resource.pure(error.asLeft)
case Right(s) => f(s).attempt(F)
}
case p @ Pure(_) =>
Resource.pure(p.a.asRight)
case e @ Eval(_) =>
Resource.eval(e.fa.attempt)
}
case p @ Pure(_) =>
Resource.pure(p.a.asRight)
case e @ Eval(_) =>
Resource.eval(e.fa.attempt)
}

def attempt(implicit F: MonadCancelThrow[F]): Resource[F, Either[Throwable, A]] =
Resource.applyFull[F, Either[Throwable, A]] { poll =>
poll(allocatedCase).attempt.map {
case Left(error) => (error.asLeft[A], _ => ().pure[F])
case Right((a, r)) => (a.asRight[Throwable], r)
lenguyenthanh marked this conversation as resolved.
Show resolved Hide resolved
}
}

@deprecated("Use overload with MonadCancelThrow", "3.6.0")
def handleErrorWith[B >: A, E](f: E => Resource[F, B])(
implicit F: ApplicativeError[F, E]): Resource[F, B] =
attempt.flatMap {
F: ApplicativeError[F, E]): Resource[F, B] =
attempt(F).flatMap {
case Right(a) => Resource.pure(a)
case Left(e) => f(e)
}

def handleErrorWith[B >: A](f: Throwable => Resource[F, B])(
implicit F: MonadCancelThrow[F]): Resource[F, B] =
attempt(F).flatMap {
Copy link
Member Author

Choose a reason for hiding this comment

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

imo, We should add a "safe" variant of handleErrorWith as well to avoid inconsistent behaviors between this and attempt.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch. You are right, we definitely should do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in 3c2d6af

lenguyenthanh marked this conversation as resolved.
Show resolved Hide resolved
case Right(a) => Resource.pure(a)
case Left(e) => f(e)
}
Expand Down Expand Up @@ -1279,7 +1302,8 @@ private[effect] trait ResourceHOInstances3 extends ResourceHOInstances4 {
}

private[effect] trait ResourceHOInstances4 extends ResourceHOInstances5 {
implicit def catsEffectMonadErrorForResource[F[_], E](
@deprecated("Use MonadCancelThrow instances", "3.6.0")
lenguyenthanh marked this conversation as resolved.
Show resolved Hide resolved
def catsEffectMonadErrorForResource[F[_], E](
implicit F0: MonadError[F, E]): MonadError[Resource[F, *], E] =
new ResourceMonadError[F, E] {
def F = F0
Expand Down Expand Up @@ -1330,10 +1354,19 @@ abstract private[effect] class ResourceFOInstances1 {
}

abstract private[effect] class ResourceMonadCancel[F[_]]
extends ResourceMonadError[F, Throwable]
extends ResourceMonad[F]
with MonadCancel[Resource[F, *], Throwable] {
implicit protected def F: MonadCancel[F, Throwable]

override def attempt[A](fa: Resource[F, A]): Resource[F, Either[Throwable, A]] =
fa.attempt(F)
lenguyenthanh marked this conversation as resolved.
Show resolved Hide resolved

def handleErrorWith[A](fa: Resource[F, A])(f: Throwable => Resource[F, A]): Resource[F, A] =
fa.handleErrorWith(f)

def raiseError[A](e: Throwable): Resource[F, A] =
Resource.raiseError[F, A, Throwable](e)

def canceled: Resource[F, Unit] = Resource.canceled

def forceR[A, B](fa: Resource[F, A])(fb: Resource[F, B]): Resource[F, B] =
Expand Down Expand Up @@ -1455,17 +1488,18 @@ abstract private[effect] class ResourceAsync[F[_]]
Resource.executionContext
}

@deprecated("Use ResourceMonadCancel", "3.6.0")
abstract private[effect] class ResourceMonadError[F[_], E]
extends ResourceMonad[F]
with MonadError[Resource[F, *], E] {

implicit protected def F: MonadError[F, E]

override def attempt[A](fa: Resource[F, A]): Resource[F, Either[E, A]] =
fa.attempt
fa.attempt(F)
Copy link
Member Author

Choose a reason for hiding this comment

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

this still used the old attempt but We can't use the new one because the error type is not fixed to Throwable.

Copy link
Member

Choose a reason for hiding this comment

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

We should mark this instance as deprecated and create a new instance that requires MonadCancelThrow.

Copy link
Member Author

@lenguyenthanh lenguyenthanh Sep 5, 2024

Choose a reason for hiding this comment

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

fixed in cb1a293. But now

private[effect] trait ResourceHOInstances4 extends ResourceHOInstances5 {
implicit def catsEffectMonadErrorForResource[F[_], E](
implicit F0: MonadError[F, E]): MonadError[Resource[F, *], E] =
new ResourceMonadError[F, E] {
def F = F0
}
}
complains because it use the deprecated ResourceMonadError.

I'm not sure what the best way to fix this. I feel like We can remove it completely. But maybe it broke client code.

Copy link
Member

Choose a reason for hiding this comment

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

We deprecate this one too :)

Copy link
Member Author

Choose a reason for hiding this comment

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

We deprecate this one but keep the trait hierarchy?

Copy link
Member

Choose a reason for hiding this comment

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

That's a good question. Maybe we should try to deprioritize this instance by moving it to the bottom of the hierarchy. Alternatively, we could just remove the implicit modifier.

Copy link
Member Author

Choose a reason for hiding this comment

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

I kept the hierachy the same but removed implicit modifier and deprecated catsEffectMonadErrorForResource function.

Copy link
Member

Choose a reason for hiding this comment

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

I thought about it more and actually I think it's okay to leave the implicit there. We don't need to change any prioritization, because we aren't introducing any new instances, and the instance derived from MonadCancelThrow (which is required for the fixed attempt) is already higher priority than this one.

By leaving it implicit, users will get an actionable deprecation instead of non-compiling code.


def handleErrorWith[A](fa: Resource[F, A])(f: E => Resource[F, A]): Resource[F, A] =
fa.handleErrorWith(f)
fa.handleErrorWith(f)(F)

def raiseError[A](e: E): Resource[F, A] =
Resource.raiseError[F, A, E](e)
Expand Down
23 changes: 23 additions & 0 deletions tests/shared/src/test/scala/cats/effect/ResourceSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1172,6 +1172,29 @@ class ResourceSpec extends BaseSpec with ScalaCheck with Discipline {
}
}

"attempt" >> {

"releases resource on error" in ticked { implicit ticker =>
IO.ref(0)
.flatMap { ref =>
val resource = Resource.make(ref.update(_ + 1))(_ => ref.update(_ + 1))
val error = Resource.raiseError[IO, Unit, Throwable](new Exception)
(resource *> error).attempt.use { _ => ref.get.map { _ must be_==(2) } }
Copy link
Member

Choose a reason for hiding this comment

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

instead of ignoring the value _ we can check that it's a Left.

}
.void must completeAs(())
}

"acquire is interruptible" in ticked { implicit ticker =>
val sleep = IO.sleep(1.second)
lenguyenthanh marked this conversation as resolved.
Show resolved Hide resolved
val timeout = 500.millis
IO.ref(false).flatMap { ref =>
val r = Resource.makeFull[IO, Unit] { poll => poll(sleep).onCancel(ref.set(true)) }(_ =>
IO.unit)
r.attempt.timeout(timeout).attempt.use_ *> ref.get
} must completeAs(true)
}
}

"uncancelable" >> {
"does not suppress errors within use" in real {
case object TestException extends RuntimeException
Expand Down
Loading