cats icon indicating copy to clipboard operation
cats copied to clipboard

Behaviour of `MonadCancelThrow[EitherT[IO, Throwable, *]]`

Open SystemFw opened this issue 3 years ago • 7 comments

This issue sounds somewhat familiar, and apologies if I forgot the exact discussion, however I stumbled on it in the wild in the context of https://github.com/typelevel/fs2/pull/2895 and found it pretty confusing.

In particular:

  • the behaviour of MonadThrow and MonadCancelThrow is inconsistent
  • the behaviour of MonadCancelThrow is weird: excluding cancelation, if something short circuits a flatMap I expect to be able to handleError it. Ofc this expectation is broken when you have two error channels, but MonadThrow works around it
val a: EitherT[IO, Throwable, Unit] =
  EitherT.leftT[IO, Unit](new Exception("Yooo"))

val console = Console[EitherT[IO, Throwable, *]]

def foo(fa: EitherT[IO, Throwable, Unit]): EitherT[IO, Throwable, Unit] =
  fa.flatMap(_ => console.println("not printed"))
    .handleError(_ => ())

def bar[F[_]: MonadThrow: Console](fa: F[Unit]): F[Unit] =
  fa.flatMap(_ => Console[F].println("not printed"))
    .handleError(_ => ())

def baz[F[_]: MonadCancelThrow: Console](fa: F[Unit]): F[Unit] =
  fa.flatMap(_ => Console[F].println("not printed"))
    .handleError(_ => ())

def x = foo(a).value.unsafeRunSync()
def y = bar(a).value.unsafeRunSync()
def z = baz(a).value.unsafeRunSync()

scala> x
val res0: Either[Throwable,Unit] = Right(())

scala> y
val res1: Either[Throwable,Unit] = Right(())

scala> z
val res2: Either[Throwable,Unit] = Left(java.lang.Exception: Yooo)

SystemFw avatar May 10 '22 00:05 SystemFw

What's the thing overriding handleErrorWith in the MonadError derivation pathway?

djspiewak avatar May 29 '22 22:05 djspiewak

reflect.runtime.universe.reify { bar(a) }val res0: reflect.runtime.universe.Expr[cats.data.EitherT[cats.effect.IO,Throwable,Unit]] = Expr[cats.data.EitherT[cats.effect.IO,Throwable,Unit]](Playground.bar(Playground.a)(EitherT.catsDataMonadErrorForEitherT(IO.asyncForIO), Console.catsEitherTConsole(IO.consoleForIO, IO.asyncForIO)))

In particular this takes precedence over this

SystemFw avatar May 30 '22 16:05 SystemFw

Oh I see what's going on. This is super annoying.

So inside of EitherTMonadCancel, we have this:

    protected def delegate: MonadError[EitherT[F, E0, *], E] =
      EitherT.catsDataMonadErrorFForEitherT[F, E, E0]

Viewed locally, this is kind of the only thing we could have (more on this in a moment), since we don't know anything specific about E0 vs E. The problem comes when E0 and E are compatible, at which point our derivation paths diverge. Cats defaults to putting the error into the innermost context (EitherT) unless it must put it in the outer, while Cats Effect always puts the error into the outermost context (F) regardless of whether it's possible to it in the inner. Thus, the derivation paths become inconsistent.

This is fixable by creating some additional disambiguation pathways all the way down the EitherT derivation chain which conditionally delegate to catsDataMonadErrorForEitherT instead of catsDataMonadErrorFForEitherT when E0 =:= E.

djspiewak avatar May 30 '22 17:05 djspiewak

Runnable reproduction.

//> using lib "org.typelevel::cats-effect:3.3.12"
//> using option "-Ykind-projector"

import cats._
import cats.data._
import cats.effect._
import cats.effect.std._
import cats.syntax.all._

import cats.effect.unsafe.implicits._

val a: EitherT[IO, Throwable, Unit] =
  EitherT.leftT[IO, Unit](new Exception("Yooo"))

val console = Console[EitherT[IO, Throwable, *]]

def foo(fa: EitherT[IO, Throwable, Unit]): EitherT[IO, Throwable, Unit] =
  fa.flatMap(_ => console.println("not printed"))
    .handleError(_ => ())

def bar[F[_]: MonadThrow: Console](fa: F[Unit]): F[Unit] =
  fa.flatMap(_ => Console[F].println("not printed"))
    .handleError(_ => ())

def baz[F[_]: MonadCancelThrow: Console](fa: F[Unit]): F[Unit] =
  fa.flatMap(_ => Console[F].println("not printed"))
    .handleError(_ => ())

def x = foo(a).value.unsafeRunSync()
def y = bar(a).value.unsafeRunSync()
def z = baz(a).value.unsafeRunSync()

@main def main =
  println(x)
  println(y)
  println(z)

armanbilge avatar Jun 12 '22 18:06 armanbilge

So I tried doing the fix as described in https://github.com/typelevel/cats/issues/4308 but quickly ran into law failures in https://github.com/typelevel/cats-effect/pull/3028. Then I tried various other things without success.

My current feeling is it's not possible to fix this by introducing another implementation of MonadCancel for EitherT; we already have the only possible implementation.

The reason Cats can have two implementations is because you can implement MonadError[EitherT] for any monadic F[_]. Thus, it provides an error channel that is completely independent of the underlying implementation.

This is not the case for MonadCancel; EitherT alone is not sufficient to implement it. You actually need a MonadCancel[F] so that you can delegate to its implementation for some methods.

The catch is that there are laws that relate the error channel of a MonadCancel datatype to its other operations. For example: https://github.com/typelevel/cats-effect/blob/9302d1ef0167575c821a8ccae46bcfd89cd15f19/laws/shared/src/main/scala/cats/effect/laws/GenSpawnLaws.scala#L120-L121

Since EitherT is not sufficient to implement fiber spawning/joining/cancellation, these operations must be delegated to the underlying F[_] which cannot "see" the error channel in EitherT. So naively raising an error into the EitherT channel will not behave consistently with the ops delegated to F[_].

I made some attempts to hack around this in https://github.com/typelevel/cats-effect/pull/3028 essentially by trying to "collapse" the two error channels into a single one, but this breaks down as well.

armanbilge avatar Jun 30 '22 04:06 armanbilge

I think @armanbilge has convinced me that this is fundamental. I think the best we can do is probably swap the precedence order in Cats (which is arbitrarily for the outer rather than the inner) to make this type of thing less jarring. In theory this would fix EitherT[IO, Throwable, *] at the cost of breaking a hypothetical IOT[Either[Throwable, *], *]. However, the latter type is impossible for IO, and it seems very likely to be similarly challenging or impossible to implement for realistic non-IO MonadCancels. For example, I think PureConc could probably do it, but does anyone use that other than me?

The problem ultimately stems from the fact that handleError does not give you a way of specifying the error channel aside from type indexing. It's just… one thing. This in turn forces raiseError to do the same, so you have to make a relatively arbitrary choice. Cats gets lucky in that all of the MonadError and ApplicativeError laws are commutative with respect to nested error channels, but Cats Effect's laws are not, which is what @armanbilge succinctly explained above. Thus, Cats can allow the decision to be arbitrary and it doesn't make a major difference unless you deconstruct the datatype, while Cats Effect cannot be so permissive.

In the event that we have a MonadError[MonadCancel[...]] (concept type, do not ingest), we must resolve any error channel type indexing in favor of the MonadCancel if the error is a subtype of Throwable. Since the inverse situation does not exist in practice, biasing inwards (as opposed to the current Cats semantic of biasing outward) should resolve the problem.

We should relocate this issue to Cats, IMO.

djspiewak avatar Sep 26 '22 11:09 djspiewak

I started looking into this, and as I suspected it is entangled with https://github.com/typelevel/cats/pull/2237.

armanbilge avatar Oct 09 '22 17:10 armanbilge