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

lenguyenthanh · 2024-09-02T18:18:54Z

This is my solution for #3757,

As I discussed with @armanbilge, the new method requires MonadCancelThrow in order to use Resource.allocatedCase which handles releasing acquired resource(s) in case of error. This pr also removed implicit ApplicativeError and deprecated the current attempt method.

~~This is a draft as it still needs to add tests and address some issues that I'll comment in the pr. But please give me early feedback.~~

Which releases acquired resource(s) in case of error and `MonadCancelThrow` constrain is required in order to do that. This also take away implicit `ApplicativeError` and deprecated the current attempt method.

lenguyenthanh · 2024-09-02T18:21:00Z

kernel/shared/src/main/scala/cats/effect/kernel/Resource.scala

@@ -1462,7 +1473,7 @@ abstract private[effect] class ResourceMonadError[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)


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

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

fixed in cb1a293. But now

cats-effect/kernel/shared/src/main/scala/cats/effect/kernel/Resource.scala

Lines 1300 to 1306 in cb1a293

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.

We deprecate this one too :)

We deprecate this one but keep the trait hierarchy?

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.

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

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.

lenguyenthanh · 2024-09-02T18:23:27Z

kernel/shared/src/main/scala/cats/effect/kernel/Resource.scala


  def handleErrorWith[B >: A, E](f: E => Resource[F, B])(
      implicit F: ApplicativeError[F, E]): Resource[F, B] =
-    attempt.flatMap {
+    attempt(F).flatMap {


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

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

fixed in 3c2d6af

armanbilge · 2024-09-03T05:47:34Z

kernel/shared/src/main/scala/cats/effect/kernel/Resource.scala

@@ -1462,7 +1473,7 @@ abstract private[effect] class ResourceMonadError[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)


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

armanbilge · 2024-09-03T05:48:22Z

kernel/shared/src/main/scala/cats/effect/kernel/Resource.scala


  def handleErrorWith[B >: A, E](f: E => Resource[F, B])(
      implicit F: ApplicativeError[F, E]): Resource[F, B] =
-    attempt.flatMap {
+    attempt(F).flatMap {


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

armanbilge · 2024-09-03T05:50:27Z

kernel/shared/src/main/scala/cats/effect/kernel/Resource.scala

+  def attempt[E](F: ApplicativeError[F, E]): Resource[F, Either[E, A]] = {
+    implicit val x: ApplicativeError[F, E] = F


We can do a trick here to automatically fix more callsites without requirng them to recompile: we can pattern match on F and if it is Sync or Async then we can use the fixed implementation instead.

I'm not sure how to implement your suggestion.

I tried:

@deprecated("Use overload with MonadCancelThrow", "3.6.0") def attempt[E](F: ApplicativeError[F, E]): Resource[F, Either[E, A]] = F match { case x: Async[_] => implicit val x: MonadCancelThrow[F] = x attempt case x: Sync[_] => attempt(x) implicit val x: MonadCancelThrow[F] = x attempt case _ => ??? }

But it doesn't work as attempt with MonadCancelThrow returns Resource[F, Either[Throwable, A]] but not Resource[F, Either[E, A]]

You can cast unsafely. If F is Sync then E must be Throwable. Also I just realized since Async <: Sync we only need to match for Sync.

Fixed it 3b7d0ee

kubukoz · 2024-09-04T16:36:50Z

kernel/shared/src/main/scala/cats/effect/kernel/Resource.scala

+
+  def attempt(implicit F: MonadCancelThrow[F]): Resource[F, Either[Throwable, A]] =
+    Resource.applyCase[F, Either[Throwable, A]] {
+      allocatedCase.attempt.map {


question: shouldn't the allocation itself be interruptible?

like Resource.applyFull { poll => poll(allocatedCase).attempt.map...

Good catch, this looks right. Let's make sure to add a test case for this.

I though about using poll but didn't know for sure. I added tests and fix this in 07fa387 and 2839446

Deprecated handleErrorWith with ApplicativeError

lenguyenthanh · 2024-09-05T19:57:58Z

core/jvm/src/main/scala/cats/effect/metrics/JvmCpuStarvationMetrics.scala

@@ -66,7 +66,7 @@ private[effect] object JvmCpuStarvationMetrics {
        case (mbeanServer, _) => IO.blocking(mbeanServer.unregisterMBean(mBeanObjectName))
      }
      .map(_._2)
-      .handleErrorWith[CpuStarvationMetrics, Throwable] { th =>
+      .handleErrorWith[CpuStarvationMetrics] { th =>


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

Fix F is `Sync`, We know that it has to be a `MonadCancelThrow`, therefore We can use safer attempt and `unsafely` cast the result.

Also remove implicit modifier from it.

armanbilge · 2024-09-08T05:55:33Z

tests/shared/src/test/scala/cats/effect/ResourceSpec.scala

+        .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) } }


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

tests/shared/src/test/scala/cats/effect/ResourceSpec.scala

kernel/shared/src/main/scala/cats/effect/kernel/Resource.scala

Co-authored-by: Arman Bilge <[email protected]>

MonadCancelThrow instance (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.

armanbilge

Looking good, one last comment :)

kernel/shared/src/main/scala/cats/effect/kernel/Resource.scala

Co-authored-by: Arman Bilge <[email protected]>

armanbilge

nice work :)

Implement a safer Resource.attempt, fix typelevel#3757

8d16f92

Which releases acquired resource(s) in case of error and `MonadCancelThrow` constrain is required in order to do that. This also take away implicit `ApplicativeError` and deprecated the current attempt method.

lenguyenthanh commented Sep 2, 2024

View reviewed changes

armanbilge reviewed Sep 3, 2024

View reviewed changes

kubukoz reviewed Sep 4, 2024

View reviewed changes

lenguyenthanh added 4 commits September 5, 2024 15:31

Add test cases for safe attempt

07fa387

Make allocatedCase interupible

2839446

Implement handleErrorWith with MonadCancelThrow

3c2d6af

Deprecated handleErrorWith with ApplicativeError

Deprecate ResourceMonadError

cb1a293

lenguyenthanh commented Sep 5, 2024

View reviewed changes

lenguyenthanh added 2 commits September 7, 2024 14:04

Best effort to improve deprecated attempt

3b7d0ee

Fix F is `Sync`, We know that it has to be a `MonadCancelThrow`, therefore We can use safer attempt and `unsafely` cast the result.

Deprecate catsEffectMonadErrorForResource

206020c

Also remove implicit modifier from it.

lenguyenthanh force-pushed the safe-attempt branch from 357cd94 to 206020c Compare September 7, 2024 12:45

lenguyenthanh marked this pull request as ready for review September 7, 2024 13:15

lenguyenthanh requested a review from armanbilge September 7, 2024 13:15

armanbilge reviewed Sep 8, 2024

View reviewed changes

lenguyenthanh and others added 4 commits September 8, 2024 15:00

Apply suggestions from code review

cd729a5

Co-authored-by: Arman Bilge <[email protected]>

Revert implicit for Resource MonadError instance

f00b9cd

MonadCancelThrow instance (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.

Verify attempt returns Left in case of error

40cb12a

Fix compilation error

840514d

armanbilge reviewed Sep 8, 2024

View reviewed changes

kernel/shared/src/main/scala/cats/effect/kernel/Resource.scala Outdated Show resolved Hide resolved

Better deprecated message

6bda660

Co-authored-by: Arman Bilge <[email protected]>

lenguyenthanh force-pushed the safe-attempt branch from 337f065 to 6bda660 Compare September 8, 2024 15:17

armanbilge approved these changes Sep 8, 2024

View reviewed changes

armanbilge requested a review from djspiewak September 8, 2024 16:23

armanbilge added the 🪲 bug label Sep 10, 2024

armanbilge added this to the v3.6.0 milestone Sep 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lenguyenthanh commented Sep 2, 2024 •

edited

Loading

lenguyenthanh Sep 2, 2024

armanbilge Sep 3, 2024

lenguyenthanh Sep 5, 2024 •

edited

Loading

armanbilge Sep 5, 2024

lenguyenthanh Sep 6, 2024

armanbilge Sep 6, 2024

lenguyenthanh Sep 7, 2024

armanbilge Sep 8, 2024

lenguyenthanh Sep 2, 2024

armanbilge Sep 3, 2024

lenguyenthanh Sep 5, 2024

armanbilge Sep 3, 2024

armanbilge Sep 3, 2024

armanbilge Sep 3, 2024

lenguyenthanh Sep 5, 2024

armanbilge Sep 5, 2024

lenguyenthanh Sep 7, 2024

kubukoz Sep 4, 2024

armanbilge Sep 4, 2024 •

edited

Loading

lenguyenthanh Sep 5, 2024

lenguyenthanh Sep 5, 2024

armanbilge Sep 8, 2024

armanbilge left a comment

armanbilge left a comment

	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
	}
	}

		def attempt[E](F: ApplicativeError[F, E]): Resource[F, Either[E, A]] = {
		implicit val x: ApplicativeError[F, E] = F

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

Are you sure you want to change the base?

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

Conversation

lenguyenthanh commented Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lenguyenthanh Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge left a comment

Choose a reason for hiding this comment

armanbilge left a comment

Choose a reason for hiding this comment

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

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

lenguyenthanh commented Sep 2, 2024 •

edited

Loading

lenguyenthanh Sep 5, 2024 •

edited

Loading

armanbilge Sep 4, 2024 •

edited

Loading