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.

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

Behaviour of MonadCancelThrow[EitherT[IO, Throwable, *]] #4308

Open
SystemFw opened this issue May 10, 2022 · 7 comments
Open

Behaviour of MonadCancelThrow[EitherT[IO, Throwable, *]] #4308

SystemFw opened this issue May 10, 2022 · 7 comments
Assignees
Milestone

Comments

@SystemFw
Copy link
Contributor

SystemFw commented May 10, 2022

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 typelevel/fs2#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)
@djspiewak
Copy link
Member

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

@SystemFw
Copy link
Contributor Author

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

@djspiewak
Copy link
Member

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.

@armanbilge
Copy link
Member

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
Copy link
Member

armanbilge commented Jun 30, 2022

So I tried doing the fix as described in #4308 but quickly ran into law failures in typelevel/cats-effect#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 typelevel/cats-effect#3028 essentially by trying to "collapse" the two error channels into a single one, but this breaks down as well.

@djspiewak
Copy link
Member

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 djspiewak transferred this issue from typelevel/cats-effect Sep 26, 2022
@armanbilge armanbilge added this to the 2.9.0 milestone Sep 26, 2022
@armanbilge armanbilge changed the title Behaviour of MonadCancelThrow[EitherT[IO, Throwable, *]] Behaviour of MonadCancelThrow[EitherT[IO, Throwable, *]] Oct 9, 2022
@armanbilge
Copy link
Member

I started looking into this, and as I suspected it is entangled with #2237.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants