Tracing reports a swallowed CancellationException warning

[CLOVIS-AI]

I'm using Arrow 1.2.3.

I am writing a simple helper for Arrow Core that introduces a Raise context in which failures throw AssertionError, with the goal of using it to simplify writing tests. Since this helper would only be used in automated tests that are expected not to fail, enabling tracing by default seems reasonable as it helps understand the root cause, so the performance impact is worth it.

Using the following code:

@ExperimentalTraceApi
inline fun <Failure, Success> failOnRaise(block: Raise<Failure>.() -> Success): Success {
    val result = either {
        traced(block) { trace, error ->
            throw AssertionError("An operation raised $error\n${trace.stackTraceToString()}")
                .apply {
                    for (suppressed in trace.suppressedExceptions())
                        addSuppressed(suppressed)
                }
        }
    }

    check(result is Either.Right) { "Impossible situation: we throw an error when the passed block raises, but it still gaves us a failed either: $result" }
    return result.value
}

// In the test
failOnRaise {
    raise(5)
}

I would expect this example to throw:

AssertionError: An operation raised `5`
arrow.core.raise.Traced: // some normal exception trace

But, instead, it throws:

java.lang.AssertionError: An operation raised 5
arrow.core.raise.Traced: kotlin.coroutines.cancellation.CancellationException should never get swallowed. Always re-throw it if captured.This swallows the exception of Arrow's Raise, and leads to unexpected behavior.When working with Arrow prefer Either.catch or arrow.core.raise.catch to automatically rethrow CancellationException.
    at arrow.core.raise.DefaultRaise.raise(Fold.kt:270)
    at opensavvy.prepared.compat.arrow.core.FailOnRaiseTest$1$3.invokeSuspend(FailOnRaiseTest.kt:21)

Is it indented that this is the error message generated by traced? If so, it's bit too worrying to be normal behavior. If not, is there something wrong on my end?

[nomisRev]

Hey @CLOVIS-AI,

That is just the standard message of the RaiseCancellationException, https://github.com/arrow-kt/arrow/blob/484c0a246a39fd1a62022ce5102472eca0f65bc3/arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/raise/Fold.kt#L306.

I am open to improving or updating it though, since you felt it was confusing here. What I find confusing is that:

public fun suppressedExceptions(): List<Throwable> =
  exception.cause?.suppressedExceptions.orEmpty() + exception.suppressedExceptions

Seems to include a reference to itself. Since what you're seeing here is the RaiseCancellationException being added through trace.suppressedExceptions(), and thus see that warning that you shouldn't swallow it. Which you're kind-of doing here.

With kind-of, I mean that you change the type of the exception and this thus by-passes the fold blocks but that is exactly your intention here 😁

So I think the bug here is that trace.suppressedExceptions() includes a reference to the original exception. If that is fixed, then trace.suppressedExceptions() should be empty and you should only see the AssertionError.

[CLOVIS-AI]

So I think the bug here is that trace.suppressedExceptions() includes a reference to the original exception. If that is fixed, then trace.suppressedExceptions() should be empty and you should only see the AssertionError.

To clarify, if this were to be done, would the initial cause still be available as part of the trace? If so, that sounds like the way to go.

[nomisRev]

To clarify, if this were to be done, would the initial cause still be available as part of the trace? If so, that sounds like the way to go.

Yes, but I think I might have spotted an edge case so let's discuss here and I'm going to involve @kyay10 since he worked on this last.

So currently we have the following behavior:

trace({
   raise("Boom") // this results in RaiseCanecllationException
   1
}) { trace, error -> trace.printStackTrace() } // <-- this shows RaiseCancellationException
          // <-- RaiseCanecllationException is rethrown

What needs to happen here:

data class OtherError(val msg: String)

either<OtherError, Int> {
  trace({
     raise("Boom") // this results in RaiseCanecllationException
     1
  }) { trace, error ->
     raise(OtherError(error)) // <-- original RaiseCancellationException String disappears,
                                              // and a new appears one for OtherError is thrown
  }

I don't think this is problematic, because that's basically what recover({ }) { } would also do, and I think we can consider the error resolved here since another one is thrown as the result of that operation.

WDYT? I think this behavior is what is expected. @CLOVIS-AI? I agree that the message in tracing is perhaps confusing, because that usage is not illegal. Sadly I don't think we can mutate the message of a Throwable, can we?

[CLOVIS-AI]

I think this behavior is what is expected. @CLOVIS-AI?

Honestly, I'm having a really hard time understanding what this all entails.

At a very high level, what I want is to provide my users with a maximally informative and minimally confusing stack-trace when a raise happens deep inside their code. As long as this objective is reached, I do not have an opinion on the implementation.

Sadly I don't think we can mutate the message of a Throwable, can we?

Let me ask another question: we agree that the message of the top-level exception thrown in this case isn't helpful in this situation. However, if it was caused by another exception, that exception and its message would be useful. My question: is the stacktrace and other details (suppressed exceptions...) from the top-level exception meaningful?

After all, I control what is displayed to the user. If everything from the exception is useful except for the message, I could just not print it. From what you've said, I'm understanding that the message is hard-coded in a way that it's 100% guaranteed it can never be anything different, so it would be safe to just ignore it, right? Or is there a risk that in some specific case the message is different and thus meaningful to the user?

[CLOVIS-AI]

Also, very minor nitpick, but the error message has missing spaces after periods :)

...Always re-throw it if captured.This swallows the exception of Arrow's Raise, and leads to unexpected behavior.When working with Arrow...

[CLOVIS-AI]

I see in Trace#stackTraceToString() that it prefers using the exception.cause over the exception itself. How does this relate with everything said here?

[CLOVIS-AI]

@nomisRev Sorry for the bump, do you know when you will have time to look at this?

If you agree with my previous comments, I will just hide the message on my side and close this: https://gitlab.com/opensavvy/groundwork/prepared/-/merge_requests/61