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.

Sign up for GitHub

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

Jump to bottom

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
Open

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

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8d16f92
Implement a safer Resource.attempt, fix #3757
lenguyenthanh Sep 2, 2024
07fa387
Add test cases for safe attempt
lenguyenthanh Sep 5, 2024
2839446
Make allocatedCase interupible
lenguyenthanh Sep 5, 2024
3c2d6af
Implement handleErrorWith with MonadCancelThrow
lenguyenthanh Sep 5, 2024
cb1a293
Deprecate ResourceMonadError
lenguyenthanh Sep 5, 2024
3b7d0ee
Best effort to improve deprecated attempt
lenguyenthanh Sep 7, 2024
206020c
Deprecate catsEffectMonadErrorForResource
lenguyenthanh Sep 7, 2024
cd729a5
Apply suggestions from code review
lenguyenthanh Sep 8, 2024
f00b9cd
Revert implicit for Resource MonadError instance
lenguyenthanh Sep 8, 2024
40cb12a
Verify attempt returns Left in case of error
lenguyenthanh Sep 8, 2024
840514d
Fix compilation error
lenguyenthanh Sep 8, 2024
6bda660
Better deprecated message
lenguyenthanh Sep 8, 2024
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
21 changes: 16 additions & 5 deletions kernel/shared/src/main/scala/cats/effect/kernel/Resource.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,9 @@ sealed abstract class Resource[F[_], +A] extends Serializable {
}
}

def attempt[E](implicit F: ApplicativeError[F, E]): Resource[F, Either[E, A]] =
@deprecated("Use overload with MonadCancelThrow", "3.6.0")
def attempt[E](F: ApplicativeError[F, E]): Resource[F, Either[E, A]] = {
implicit val x: ApplicativeError[F, E] = F
Copy link
Member

Choose a reason for hiding this comment

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

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.

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'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]]

Copy link
Member

Choose a reason for hiding this comment

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

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.

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 it 3b7d0ee

this match {
case Allocate(resource) =>
Resource.applyFull { poll =>
Expand All @@ -710,19 +712,28 @@ sealed abstract class Resource[F[_], +A] extends Serializable {
}
}
case Bind(source, f) =>
Resource.unit.flatMap(_ => source.attempt).flatMap {
Resource.unit.flatMap(_ => source.attempt(F)).flatMap {
case Left(error) => Resource.pure(error.asLeft)
case Right(s) => f(s).attempt
case Right(s) => f(s).attempt(F)
}
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.applyCase[F, Either[Throwable, A]] {
allocatedCase.attempt.map {
Copy link
Member

Choose a reason for hiding this comment

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

question: shouldn't the allocation itself be interruptible?

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

Copy link
Member

@armanbilge armanbilge Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

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 though about using poll but didn't know for sure. I added tests and fix this in 07fa387 and 2839446

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

def handleErrorWith[B >: A, E](f: E => Resource[F, B])(
implicit F: ApplicativeError[F, E]): Resource[F, B] =
attempt.flatMap {
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 @@ -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)
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 •
edited
Loading

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

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.

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)
Expand Down
Loading