Suppressed exceptions can result in some exception...

# arrow-contributors

[Youssef Shoaib [MOD]]

Suppressed exceptions can result in some exceptions disappearing if they were attached to `RaiseCancellationException`:

impure {
  resourceScope {
    onRelease { throw RuntimeException("foo") }
    onRelease { raise() }
  }
} // Never throws

[Youssef Shoaib [MOD]]

Maybe fold should rethrow suppressed exceptions? We need to be careful though because of the following 2 cases:

impure {
  resourceScope {
     onRelease { raise() }
     onRelease { raise() }
   }
 }

impure outer@{
  impure {
    resourceScope {
      onRelease { raise() }
      onRelease { outer.raise() }
    }
  }
}

[simon.vergauwen]

Oof, good find! raise from onRelease is kind-of weird though.. Should we e.printStackTrace, send it to CoroutineExceptionHandler, or?

[Youssef Shoaib [MOD]]

It doesn't have to be in onRelease...

impure {
  resourceScope {
    onRelease { throw RuntimeException("foo") }
    raise()
  }
} // Never throws

Although, yes, if we assume that raise in onRelease is a bad pattern, then we can easily fix this issue by rethrowing suppressed exceptions in fold.

[Youssef Shoaib [MOD]]

More food for thought: `CancellationException`s are practically used for control flow manipulation. With the normal tools of control flow we have (return, break, continue), a finalizer exception takes priority over them. Thus, when merging exceptions in ResourceScopeImpl (and AutocloseScopeImpl), we should maybe give priority to non-`CancellationException`s, perhaps tacking on the CancellationException as a suppressed one.

[Youssef Shoaib [MOD]]

The way we handle suppressions now is the correct Java way, in which exceptions in try take priority over exceptions in close. This results in an ordering on exceptions where exceptions in the resourceScope block take priority, then exceptions in onRelease are LIFO ordered.
This matches the Java system because something like:

resourceScope {
  val x = a.bind()
  val y = b.bind()
  throw c
}

should behave the same as (excuse my bad Java):

try(var x = a) {
  try(var y = b) {
    throw c
  }
}
// which behaves the same as
try(var x = a;
    var y = b) {
  throw c
}

and indeed, the rule that try triumphs over close implies that ordering. If we remove throw c, then in all the examples, `b`'s exception is the one thrown, and `a`'s exception is added to it as a suppressed exception.
What I'm suggesting is changing that order a little bit: CancellationException should rank below other exceptions, and then we sort as per normal. After all, `raise`d errors are logical, meaning they're expected, while Exceptions are unexpected, and thus should be propagated.
WDYT @simon.vergauwen? I basically don't want to end up with a CancellationException that has any suppressed exceptions, unless those suppressed exceptions are also `CancellationException`s

[Youssef Shoaib [MOD]]

Interestingly, Scala's ControlThrowable says:
> Suppression is disabled, because flow control should not suppress an exceptional condition
I'm not sure what suppression disabling does exactly, but maybe it's exactly what we need here.
Edit: I tested disabling suppression in Java, and it seems to just not keep track of suppressed exceptions, but it doesn't change the priority or anything. It's a bit disappointing. I wonder if Scala somehow gets around this (after all, they have `boundary`/`break`, which is Raise under a different name and worse API)

[Youssef Shoaib [MOD]]

The plot thickens... Scala had this reported, and they changed their `try-with-resources`/`resourceScope` equivalent to deprioritize ControlThrowble below all else! I think that's good precedent for us to do the same (and the stdlib should probably do the same for use).

In fact, they have a very well defined suppression order:

* ==Suppression Behavior==
  *
  * If two exceptions are thrown (e.g., by an operation and closing a resource),
  * one of them is re-thrown, and the other is
  * [[java.lang.Throwable#addSuppressed added to it as a suppressed exception]].
  * If the two exceptions are of different 'severities' (see below), the one of a higher
  * severity is re-thrown, and the one of a lower severity is added to it as a suppressed
  * exception. If the two exceptions are of the same severity, the one thrown first is
  * re-thrown, and the one thrown second is added to it as a suppressed exception.
  * If an exception is a [[scala.util.control.ControlThrowable `ControlThrowable`]], or
  * if it does not support suppression (see
  * [[java.lang.Throwable `Throwable`'s constructor with an `enableSuppression` parameter]]),
  * an exception that would have been suppressed is instead discarded.
  *
  * Exceptions are ranked from highest to lowest severity as follows:
  *   - `java.lang.VirtualMachineError`
  *   - `java.lang.LinkageError`
  *   - `java.lang.InterruptedException` and `java.lang.ThreadDeath`
  *   - [[scala.util.control.NonFatal fatal exceptions]], excluding `scala.util.control.ControlThrowable`
  *   - all other exceptions, excluding `scala.util.control.ControlThrowable`
  *   - `scala.util.control.ControlThrowable`
  *
  * When more than two exceptions are thrown, the first two are combined and
  * re-thrown as described above, and each successive exception thrown is combined
  * as it is thrown.
  *

I don't know if we should go all-out and implement the full ordering, but at the very least, deprioritising CancellationException definitely seems like a good idea here. The stdlib definitely should do the same (and Java itself probably should as well, since they have CancellationException)

EDIT: hmm, it turns out boundary/break doesn't use ControlThrowable for some complicated reasons I'm yet to understand. This means that the suppression behaviour is broken for it, just like it is for raise. I need a coffee to figure this mess out! I think ultimately, CancellationException behaves similarly to ControlThrowable , and so should be handled the same way. The fact that boundary/break doesn't use ControlThrowable is irrelevant because raise uses CancellationException , if that makes any sense.
Part of the Scala rationale for not using ControlThrowable seems to be because their version of runCatching didn't catch it, and that was bad because they were using runCatching in the same way to how coroutines uses it: to pass around the state of execution from one thread to another. I think they might be "wrong" here though: all exceptions should be passed around between coroutines, regardless of their fatality. That's what Kotlin does right now, and that's what Result implies. The issues they faced with ControlThrowable are thus irrelevant to us, I think. This is all just my current thoughts, so take it with a grain of salt.
To add to the complications, Scala doesn't seem to care about CancellationException at all! It treats it like any other nonfatal exception.