add support for catching failures only, without defects #9488
Conversation
df279cb to
71a6336
Compare
There was a problem hiding this comment.
I really appreciate this contribution, although I'm a bit sceptical because I feel like it will make things more confusing around effectful error handling.
I think the method signature should be changed to Cause[E] => ... instead of Cause.Fail[E] => .... However, doing so will definitely make things more conf
8000
using for users and zio-* library authors since we already have methods with similar signature / naming. If we're going to merge this in we need to make it extremely clear of the differences in the scaladoc of both the new methods but also the old ones
| final def catchSomeFailure[R1 <: R, E1 >: E, A1 >: A]( | ||
| pf: PartialFunction[Cause.Fail[E], ZIO[R1, E1, A1]] | ||
| )(implicit trace: Trace): ZIO[R1, E1, A1] = { |
| * {{{ | ||
| * effect.catchAllFailure(_ => backup()) | ||
| * }}} |
There was a problem hiding this comment.
This is going to be very confusing to users on why to use this method over catchAllZIO. We need to provide a better example in the scaladoc that differentiates, but also point to catchAllZIO when the users don't care about retrieving the full Cause of the failure (which is in most cases in user code)
There was a problem hiding this comment.
You meant catchAllCause or catchAll?
There was a problem hiding this comment.
documentation improved.
| /** | ||
| * Returns an effect that effectfully "peeks" at the failure of this effect. | ||
| * {{{ | ||
| * readFile("data.json").tapError(logError(_)) | ||
| * }}} | ||
| */ |
|
Moving to Cause[E] removes the ability to access the actual error in an easy way. For example, with Cause.Fail[E], you can: However, you cannot do that with Cause[E]. I think that the current names and documentation are a bit confusing. For instance,
However, failure should not include defects. Similarly, the name
But do errors include defects? The documentation is not clear about the fact that it also catches defects. On the other hand, we have tapDefect and catchAllDefect, which are better documented. The reason all of this came up is that we are in the process of upgrading to ZIO 2.0. Previously, we used catchAll and tapError to log errors using zio-logging's This is why I think Interestingly, the
A similar warning should also exist for |
|
@hearnadam @kyri-petrou can someone please take a look at this again? |
|
Just a note to say that everything @somdoron said matches my experience. The existing implicit final class RichZIO[-R, +E, +A](private val self: ZIO[R, E, A]) extends AnyVal {
final def catchAllFailureCause[R1, E1, A1 >: A](
f: (E, Cause[E]) => ZIO[R1, E1, A1]
)(
implicit trace: Trace
): ZIO[R & R1, E1, A1] =
self.catchAllCause { cause =>
cause.failureOrCause match {
case Left(e) => f(e, cause)
case Right(c) => ZIO.refailCause(c)
}
}
final def catchSomeFailureCause[RR, EE >: E, AA >: A](
pf: PartialFunction[(E, Cause[E]), ZIO[RR, EE, AA]]
)(
implicit trace: Trace
): ZIO[R with RR, EE, AA] =
self.catchSomeCause { cause =>
cause.failureOrCause match {
case Left(e) => pf.applyOrElse[(E, Cause[E]), ZIO[RR, EE, AA]]((e, cause), _ => ZIO.refailCause(cause))
case Right(c) => ZIO.refailCause(c)
}
}
final def tapFailureCause[R1, E1 >: E](
f: (E, Cause[E]) => ZIO[R1, E1, Any]
)(
implicit trace: Trace
): ZIO[R & R1, E1, A] =
self.tapErrorCause { cause =>
cause.failureOption match {
case Some(e) => f(e, cause)
case None => ZIO.refailCause(cause)
}
}
} |
| * effect.catchAllFailure(c => ZIO.logErrorCause("failure", c)) | ||
| * }}} | ||
| */ | ||
| final def catchAllFailure[R1 <: R, E1 >: E, A1 >: A]( |
There was a problem hiding this comment.
What about naming this catchFailureCause instead? I think that would be a more accurate naming for this method right?
There was a problem hiding this comment.
I made the change, but I would this is creating a new convention, as for now, all of the overloads start with with catchAll:
catchAll
catchAllTrace
catchAllCause
catchAllDefect
| * } | ||
| * }}} | ||
| */ | ||
| final def catchSomeFailure[R1 <: R, E1 >: E, A1 >: A]( |
There was a problem hiding this comment.
As above, it's a bit more verbose but what about catchSomeFailureCause?
| * readFile("data.json").tapError(logError(_)) | ||
| * }}} | ||
| */ | ||
| final def tapFailure[R1 <: R, E1 >: E]( |
There was a problem hiding this comment.
What are your thoughts on tapFailureCause
|
@kyri-petrou I’ve applied the changes you suggested. There’s one thing I’m not entirely happy with regarding this set of methods: the type is For example, you’d have to write something like: Which feels a bit clunky. An alternative would be to use At the moment, the new methods provide access to all failures. @kyri-petrou what do you think? @quelgar what are your thoughts on that? |
|
@somdoron yes that makes sense to me. If all we got was a |
There was a problem hiding this comment.
I'm really sorry for taking this long to get to review this. Every time I started reviewing it I went into a rabbit-hole with trying to figure out potential footguns for users, how to minimize API bloating and more importantly how to make these methods as maintainable as possible.
Frankly, I prefer this one over #9738 as #9738 brings too much API bloating around logging.
| )(implicit ev: CanFail[E], trace: Trace): ZIO[R1, E1, A1] = { | ||
| def tryRescue(c: Cause[E]): ZIO[R1, E1, A1] = | ||
| if (c.isFailureOnly) { | ||
| c.find { case f: Cause.Fail[E] => f } match { |
There was a problem hiding this comment.
I feel like pf.isDefinedAt should be used in this partial function no?
| { case (e, stackTrace) => | ||
| h(Cause.Fail(e, stackTrace)) | ||
| }, |
There was a problem hiding this comment.
If you were to change the signature, you could use failureOrCause here
| * }}} | ||
| */ | ||
| final def catchFailureCause[R1 <: R, E2, A1 >: A]( | ||
| h: Cause.Fail[E] => ZIO[R1, E2, A1] |
There was a problem hiding this comment.
I still think this should be Cause[E] => ZIO[R1, E2, A1]
| * }}} | ||
| */ | ||
| final def catchSomeFailureCause[R1 <: R, E1 >: E, A1 >: A]( | ||
| pf: PartialFunction[Cause.Fail[E], ZIO[R1, E1, A1]] |
There was a problem hiding this comment.
As above, I think we need to change Cause.Fail[E] to Cause[E]
| * }}} | ||
| */ | ||
| final def tapFailureCause[R1 <: R, E1 >: E]( | ||
| f: Cause.Fail[E] => ZIO[R1, E1, Any] |
| @@ -412,6 +415,28 @@ sealed abstract class Cause[+E] extends Product with Serializable { self => | |||
| (causeOption, stackless) => causeOption.map(Stackless(_, stackless)) | |||
| ) | |||
|
|
|||
| def keepFailures: Option[Cause[E]] = | |||
There was a problem hiding this comment.
Do you still think this one is needed?
fixes #9485.
ZIO 2.0 removed zio-logging
logThrowable, ZIO 2.0 can only log causes. When one want to log an error, callingcatchAllortapErroris not enough, because those cannot be logged. Therefore one must usecatchAllCauseortapErrorCause, however those catch defects as well, which usually when recovering you don't actually want to catch. Currently there is no way to catch errors as cause without the defects.This PR adds new variants to allow catching errors as cause without defects, which would allow them to be logged.