Isn't it a bit too dangerous to catch a Throwable ...

# arrow

[kartoffelsup]

Isn't it a bit too dangerous to catch a Throwable in Try {}? I think this is why I had several NoClassDefFound Errors in my sample above (https://kotlinlang.slack.com/archives/C5UPMM0A0/p1550165387048700), because a StackOverFlowError was being caught.
https://github.com/arrow-kt/arrow/blob/e82fcf69a35688584e3fb4e8e4ff44a44675c551/modules/core/arrow-core-data/src/main/kotlin/arrow/core/Try.kt#L39

[raulraja]

Yes, Arrow does not yet have the concept of NonFatal which we need to avoid catching errors like SOE Linkage errors etc that the user can't recover from.

[raulraja]

https://github.com/arrow-kt/arrow/issues/153

[raulraja]

We need help with that. If anyone wants to take a shot at it I can help pointing out all relevant places in the code base that would need to be updated.

[raulraja]

Esentially all the error handling data types and type classes that do error control including those in the Fx modules

[raulraja]

I'm also of the opinion that Try should not be part of Arrow and I plan to make a proposal previous to 1.0 to get rid of it

[kartoffelsup]

Thanks for the explanation! I would be interested in helping out, but I'm not very experienced in FP or Scala. And also thanks for Arrow :)

[raulraja]

@kartoffelsup no problem and thanks!. Regarding NonFatal is essentially porting this to Kotlin https://github.com/scala/scala/blob/v2.9.3/src/library/scala/util/control/NonFatal.scala#L1

[raulraja]

In Kotlin there is no destructuring pattern matching but it can be done with regular when. It could be as easy as implementing something like:

internal fun <A> Throwable.nonFatal(): NonFatal =
  if (isNonFatal()) NonFatal(this) else throw this

[kartoffelsup]

Does arrow need the 'latest' Version (from 2.13.x) which treats StackOverFlowErrors as Fatal as well? https://github.com/scala/scala/blob/2.13.x/src/library/scala/util/control/NonFatal.scala https://github.com/scala/scala/commit/64603653f82082431941cb29ad317b669822f28e

[raulraja]

then refactor the exception handlers to capture only non fatal exceptions and let them propagate Fatal ones.

[raulraja]

Yes, the link I posted is from an old version

[raulraja]

SOEs are fatal

[kartoffelsup]

Okay, I've went ahead and just 'copied' the Scala version into Arrow

kotlin
/**
 * Extractor of non-fatal Throwables. Will not match fatal errors like `VirtualMachineError`
 * (for example, `OutOfMemoryError` and `StackOverflowError`, subclasses of `VirtualMachineError`), `ThreadDeath`,
 * `LinkageError`, `InterruptedException`.
 */
object NonFatal {
  /**
   * @return true if the provided `Throwable` is to be considered non-fatal, or false if it is to be considered fatal
   */
  operator fun invoke(t: Throwable): Boolean =
    when (t) {
      is VirtualMachineError, is ThreadDeath, is InterruptedException, is LinkageError -> false
      else -> true
    }
}

And updated all production code places that catch a Throwable to if(NonFatal(t)) [ doIt } { else throw t}
or do you prefer the Throwable extension function?

[raulraja]

awesome, we can have both

[raulraja]

this is useful to users not just us internally

[raulraja]

fun Throwable.nonFatal(): Throwable =
  if NonFatal(this) this else throw this

[raulraja]

based on your implementation ^^^

[raulraja]

that allows anyone with a throwable in their hand to let it bubble up

[kartoffelsup]

Alright, I will pull request this in a couple of moments. 🙂 Thanks for the help!

[raulraja]

should be used in all raiseError implementations and places that currently capture a Throwable via try / catch

[raulraja]

Potentially coroutines exceptions handlers as well in MonadErrorContinuation

[raulraja]

@kartoffelsup thanks so much!

[kartoffelsup]

https://github.com/arrow-kt/arrow/pull/1305

[raulraja]

Alright, all reviewed and looks great

[raulraja]

left comments for improvements which will push this feature to all data types and type classes in the MonadThrow hierarchy

[dmcg]

Just lurking, but interested in why NonFatal is an object and not a top level function?

[kartoffelsup]

Good question. I guess I only focused on copying the Scala version. A top level function would've sufficed though. I will change that once I'm home. Thanks! arrow

[dmcg]

I never know whether there are important subtleties that I’ve missed!