Alternative implementation for Catch

jatcwang commented 1 week ago

Hi thanks for the library! This isn't an issue but more of a curiosity :) I came up with a slightly different implementation given the original API and I'm wondering whether it's something you've considered

In my implementation, I'm canceling the fiber from "outside" the fiber instead of inside. When raise is called, I use IO.never to pause the execution.

override def raise[A](io: Abort[E] => IO[A]): IO[Either[E, A]] = {
    for {
      errorChannel <- Deferred[IO, E]
      raise = new Abort[E] {
        override def apply[AA](e: E): IO[AA] = 
           errorChannel.complete(e) *> IO.never
      }
      fiber <- io(using raise).start
      res <- errorChannel.get.race(fiber.joinWithNever)
    } yield res

So I'm wondering whether you tried this approach and whether there were drawbacks which lead to the current design. Although I don't think it matters much except that we don't need to handle the case we're in an uncancellable section.

ValdemarGr commented 1 week ago

Thanks for the curiosity :)

It has been a while ago since I had all the state in my head, but here are some of the considerations I remember. If you see any fault in my interpretation of the problem then please let me know!

Infinite cancelation

This section is the only important one to answer your question, the rest is appendix. Consider the following program:

val r: Raise[IO, String] = ???
IO.uncancelable(_ => r.raise((a: Abort[String]) => a("err")))

The program will never terminate since IO.never occurs in a uncancelable block. Using the Raise algebra inside of a uncancelable is a bug, so the behavior is not well-defined.

Here is a more complete example.

```scala import cats.effect._ import cats.implicits._ import scala.concurrent.duration._ object Main extends IOApp.Simple { def run = IO.deferred[String].flatMap { errorChannel => val r = Resource.make(IO.unit)(_ => errorChannel.complete("err") *> IO.never) IO.println("starting") *> IO.race( r.use_, IO.sleep(2.seconds) *> IO.println("timeout") ).void } } ```

I think this behavior is very hard to debug, so I think an exception is better.

Bi-directional cancelation

I cancel in both directions (self-cancel from raise and the closest handle) to alleviate the previous section. If the cancelation only traveled in one direction some issues occur:

If `raise` is cancelled by it's parent via a `Deferred` (your example).

There is a small race condition between fiber that is scheduled to evaluate

errorChannel.get.race(fiber.joinWithNever)

and whatever happens after

errorChannel.complete(e)

Adding IO.never moves us back the the previous section.

If `raise` cancels itself and no parent cancelation occurs

The self-cancelled fiber may not be the same fiber that is managed by the closest handle instance. For instance, in

IO.race(
  fa.flatMap(raise).onCancel(IO.pure(42)), // race_1 fiber
  fb // race_2 fiber
).start // <- root fiber from raise

fa.flatMap(raise) will recover from cancelation, such that the cancelation will not propagate until the nearest handle is found.

Cancelling from both directions

Say that an error was raised within a uncancelable block in the current solution of catch-effect, then the closest handle instance should cancel it's running fiber. But now we are subject to the same type of race-condition from the above section, since there may be instructions that are run on the raising fiber which should have been cancelled.

Code

```scala import cats.effect._ import cats.implicits._ import scala.concurrent.duration._ object Main extends IOApp.Simple { def run = IO.uncancelable(_ => IO.canceled *> IO.raiseError(new Exception("cancel!"))).start.flatMap(_.joinWithNever).attempt .map(println(_)) // This side effect runs even though we raise! } ```

The choice is ultimately:

Block for ever.
Raise an error but be subject to catch-all exception handlers.

A stray though on error channels

I think a very interesting idea to explore could be how would an effect system look if ad-hoc error channel introduction was part of the effect system. Maybe the channel for exceptions (whatever is required for IO to form a MonadThrow and most of the cats-effect typeclasses) could be introduced as an instance of something more fundamental. I think having orthogonal error channels being a abstraction that the effect system provides could lead to some of these cancelation issues disappearing (or at-least having a more reasonable solution).

case class GetCatch[E]() extends IO[Handle[E]]

// this is a implementation of GetCatch that CE provides for convenience
case class ThrowableChannel() extends IO[Handle[Throwable]]

object IO {
  def throwableChannel: IO[Handle[Throwable]] = ThrowableChannel()
}

new MonadThrow[IO] {
  // ...
  def raiseError[A](e: Throwable): IO[A] = IO.throwableChannel.flatMap(_.raise(e))
  def attempt[A](fa: IO[A]): IO[Either[Throwable, A]] = IO.throwableChannel.flatMap(ec => ec.attempt(fa))
}

jatcwang commented 6 days ago

Thanks for the detailed explanation. I think I do understand now why we need bidirectional cancellation! And yeah it makes sense to forbid using raise inside a uncancellable block.

Interesting thought on MonadError/MonadThrow too with a dedicated error channel that's definitely sounding like ZIO :laughing: Wouldn't be surprised if that's what they're doing with their cats effect integration

ValdemarGr / catch-effect