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

Make IO#onError consistent with ApplicativeError #4121

Merged
merged 8 commits into from
Aug 28, 2024
Merged

Make IO#onError consistent with ApplicativeError #4121

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
932fa7d
Make IO.onError consistent with Applicative.onError
lenguyenthanh Aug 20, 2024
b4cd5e5
Replace usage of the old onError with the new one
lenguyenthanh Aug 20, 2024
7825274
Make scala 2.12 happy
lenguyenthanh Aug 20, 2024
f4f505c
Reuse logic between IO.onError
lenguyenthanh Aug 21, 2024
40a7dde
Add reportError helper and use it for onError
lenguyenthanh Aug 22, 2024
4764d05
Use reportError when applicable
lenguyenthanh Aug 22, 2024
868451b
Fix onError callback error handling semantic
lenguyenthanh Aug 22, 2024
c6026b3
Override onError for IOAsync
lenguyenthanh Aug 26, 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
47 changes: 35 additions & 12 deletions core/shared/src/main/scala/cats/effect/IO.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,11 +489,8 @@ sealed abstract class IO[+A] private () extends IOPlatform[A] {
def guarantee(finalizer: IO[Unit]): IO[A] =
// this is a little faster than the default implementation, which helps Resource
IO uncancelable { poll =>
val handled = finalizer handleErrorWith { t =>
IO.executionContext.flatMap(ec => IO(ec.reportFailure(t)))
}

poll(this).onCancel(finalizer).onError(_ => handled).flatTap(_ => finalizer)
val onError: PartialFunction[Throwable, IO[Unit]] = { case _ => finalizer.reportError }
poll(this).onCancel(finalizer).onError(onError).flatTap(_ => finalizer)
}

/**
Expand All @@ -519,12 +516,10 @@ sealed abstract class IO[+A] private () extends IOPlatform[A] {
def guaranteeCase(finalizer: OutcomeIO[A @uncheckedVariance] => IO[Unit]): IO[A] =
IO.uncancelable { poll =>
val finalized = poll(this).onCancel(finalizer(Outcome.canceled))
val handled = finalized.onError { e =>
finalizer(Outcome.errored(e)).handleErrorWith { t =>
IO.executionContext.flatMap(ec => IO(ec.reportFailure(t)))
}
val onError: PartialFunction[Throwable, IO[Unit]] = {
case e => finalizer(Outcome.errored(e)).reportError
}
handled.flatTap(a => finalizer(Outcome.succeeded(IO.pure(a))))
finalized.onError(onError).flatTap { (a: A) => finalizer(Outcome.succeeded(IO.pure(a))) }
}

def handleError[B >: A](f: Throwable => B): IO[B] =
Expand Down Expand Up @@ -588,8 +583,20 @@ sealed abstract class IO[+A] private () extends IOPlatform[A] {
def onCancel(fin: IO[Unit]): IO[A] =
IO.OnCancel(this, fin)

def onError(f: Throwable => IO[Unit]): IO[A] =
handleErrorWith(t => f(t).voidError *> IO.raiseError(t))
@deprecated("Use onError with PartialFunction argument", "3.6.0")
def onError(f: Throwable => IO[Unit]): IO[A] = {
val pf: PartialFunction[Throwable, IO[Unit]] = { case t => f(t).reportError }
onError(pf)
}

/**
* Execute a callback on certain errors, then rethrow them. Any non matching error is rethrown
* as well.
*
* Implements `ApplicativeError.onError`.
*/
def onError(pf: PartialFunction[Throwable, IO[Unit]]): IO[A] =
handleErrorWith(t => pf.applyOrElse(t, (_: Throwable) => IO.unit) *> IO.raiseError(t))
Copy link
Member

Choose a reason for hiding this comment

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

To match the other implementation we need this voidError. But this raises an interesting question about the semantics: if the onError callback raises an error, which error should be propagated, that error or the original one? Also the voidError completely discards the error without reporting it, which seems really weird 😕

Suggested change
handleErrorWith(t => pf.applyOrElse(t, (_: Throwable) => IO.unit) *> IO.raiseError(t))
handleErrorWith(t => pf.applyOrElse(t, (_: Throwable) => IO.unit).voidError *> IO.raiseError(t))

Copy link
Member

Choose a reason for hiding this comment

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

I think what we should do is extract this into a helper method.

cats-effect/core/shared/src/main/scala/cats/effect/IO.scala

Line 493 in 7168625

IO.executionContext.flatMap(ec => IO(ec.reportFailure(t)))

Then instead of voidError we should redirect errors within the onError callback to that? And then re-raise the original error.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks @armanbilge, I'm completely missed that voidError. I added reportError helper method as your suggestion. I think this is a great improvement. The name is subjecting for bike-shedding. We can also use this reportError method in the two other places. I can do that in this pr.

Copy link
Member

Choose a reason for hiding this comment

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

I'm completely missed that voidError

Yes, I almost missed it as well. We should add a test to cover this case (what happens if onError callback raises an error), that would have helped us. Actually we might even consider adding this as a law to Cats, so that onError semantics are the same across all implementations.

We can also use this reportError method in the two other places. I can do that in this pr.

🚀 it looks good! I think name is reasonable and it is private internal anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

cool, I'll add a test case for in this first. And think about a law for cats later on.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, actually, now I am hesitating 😅

The goal of this PR is to make IO#onError consistent with ApplicativeError#onError. If we use voidError/reportError then the semantics are inconsistent again with ApplicativeError 😕

https://github.com/typelevel/cats/blob/8e8724a29881a394bc39357a7f0cd21124573f30/core/src/main/scala/cats/ApplicativeError.scala#L261-L262

Unless we change it in Cats (and this may be too drastic of a change) I think we just have to let the callback's error propagate 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.

ahhhh, you're right. So, my first implemenaton was accidentally correct :p.


/**
* Like `Parallel.parProductL`
Expand Down Expand Up @@ -928,6 +935,19 @@ sealed abstract class IO[+A] private () extends IOPlatform[A] {
def void: IO[Unit] =
map(_ => ())

/**
* Similar to [[IO.voidError]], but also reports the error.
*/
private[effect] def reportError(implicit ev: A <:< Unit): IO[Unit] = {
val _ = ev
asInstanceOf[IO[Unit]].handleErrorWith { t =>
IO.executionContext.flatMap(ec => IO(ec.reportFailure(t)))
}
}

/**
* Discard any error raised by the source.
*/
def voidError(implicit ev: A <:< Unit): IO[Unit] = {
val _ = ev
asInstanceOf[IO[Unit]].handleError(_ => ())
Expand Down Expand Up @@ -1975,6 +1995,9 @@ object IO extends IOCompanionPlatform with IOLowPriorityImplicits with TuplePara
override def handleError[A](fa: IO[A])(f: Throwable => A): IO[A] =
fa.handleError(f)

override def onError[A](fa: IO[A])(pf: PartialFunction[Throwable, IO[Unit]]): IO[A] =
fa.onError(pf)

override def timeout[A](fa: IO[A], duration: FiniteDuration)(
implicit ev: TimeoutException <:< Throwable): IO[A] = {
fa.timeout(duration)
Expand Down
Loading