There's a scary edge case in Arrow that can mean r...

# arrow-contributors

[Youssef Shoaib [MOD]]

There's a scary edge case in Arrow that can mean resource-closing exceptions go unnoticed and unreported...
Even worse, Kotlin stdlib suffers from the same issue, too (through use)
I'm aiming to fix this once and for all!
Probably my highest PR description-to-diff ratio yet!

[simon.vergauwen]

cc\\ @CLOVIS would love your thoughts on this

[CLOVIS]

I read it, I agree disappearing errors are scary, but I'm not really sure I get the impacts this change could have on production code.

[Youssef Shoaib [MOD]]

In some cases, code that worked just fine would now throw exceptions. The exceptions were always introduced, but they were suppressed by CancellationException, then kotlinx-coroutines or Raise would recover from the cancellation, but thus continue ignoring the original error.

[CLOVIS]

Right, but did it really ever work fine if it swallowed errors?

[Youssef Shoaib [MOD]]

That's exactly my argument.
Imagine the resource cleanup action was persistenting some data to disk. Before, persistence might throw exceptions, and they get ignored. With this change, you'd start getting these errors.
This is especially potent for saga. Compensation actions could be failing, and the code never finds out about it. It feels like a break of contract (not saying it actually is, just that it feels weird)

[simon.vergauwen]

It's a bit more nuanced though. That only happens when cancellation comes first.

[simon.vergauwen]

The exceptions get "added" to the cancellation, but cancellation throws them away...

[Youssef Shoaib [MOD]]

Sure, but "cancellation" can just be a raise, so it's not just coroutine shutdown

[simon.vergauwen]

Okay, but maybe we need to consider something specifically for raise then? That's exactly why I gave the raise exception a public api so you can distinct between "normal cancellation" and raise.

[Youssef Shoaib [MOD]]

Sure, that could be a good distinction to make here. Maybe RaiseCancellationException is what should be deprioritized, not CancellationException
I'm a little bit skeptical just because CancellationException sometimes gets thrown not because of coroutine shutdown (e.g. if you cancel a Deferred then try to await it), but this can be a good compromise