PSA; don't try-catch `IllegalStateException` becau...

# coroutines

[Paul Woitaschek]

PSA; don't try-catch IllegalStateException because a CancellationException is a subclass and it puts your application into an unexpected state if you catch it. Just found out the hard way

[Dico]

Why does it subclass that?

[Paul Woitaschek]

Well kotlinx.coroutines.CancellationException is a typealias of java.util.concurrent.CancellationException and that extends java.lang.IllegalStateException

[Paul Woitaschek]

I have shot myself so often in my foot by accidentially swallowing CancellationException 😕

[Paul Woitaschek]

@elizarov Can't coroutine have their own cancellation exception? This makes error handling really hard, especially as functions like error also throw IllegalStateExeption

[Adam Powell]

under what circumstances would you specifically try-catch IllegalStateException? Making assumptions about how to recover from one vs. Exception or Throwable seems quite brittle since you don't know what threw it or why without further info, like a more specific exception type

[Dico]

any suspend function that may throw IllegalStateException?

[Paul Woitaschek]

I'm working with the samsung health sdk and it randomly throws illegalstateexceptions.

[Adam Powell]

@Dico unless you're making unsafe assumptions about what that ISE means it's not really meaningful to catch it any more specifically than Throwable

[Dico]

We're talking about a very specific use case, you're talking about generalizing strategy, that is not useful

[Dico]

The point is that functions can and will throw concretely IllegalStateException and catching it interferes with coroutines machinery unintentionally

[Paul Woitaschek]

@Adam Powell Don't you agree that the coroutine machinery for cancellation interferes with exception throwing code in an error prone way?

[Adam Powell]

not in a way that is specific to IllegalStateException. Job cancellation exists at the library layer, not a language layer. The mechanism for stack unwinding in such circumstances is to throw. catch (t: Throwable) has the same gotcha and requires the same consideration, but given the rest of the advantages of the design imo it's worth the tradeoffs

[Adam Powell]

changing the exception hierarchy that it uses at this point for it would be a breaking change so I'm guessing it's not going to happen

[Paul Woitaschek]

I see it as a severe flow and something that you constantly have to be careful with

[Adam Powell]

Now I'm also curious about the specific samsung health sdk method you're referring to; do you have a link to some public docs by any chance?

[Paul Woitaschek]

I don't think that's relevant here. It's simple code that gets broken in a subtle way

[Paul Woitaschek]

For example if you take this code:

try {
        thirdPartyApi.doSomethingThatCanThrow()
    } catch (e: IllegalStateException) {
        e.printStackTrace()
    }

[Paul Woitaschek]

Now you add a simple delay and you have sucessfully shot yourself in the foot:

try {
        thirdPartyApi.doSomethingThatCanThrow()
        delay(100)
    } catch (e: IllegalStateException) {
        e.printStackTrace()
    }

[elizarov]

Don’t catch ISEs. Period. (*also applies to NPEs, OOMs, etc). This simple rule will save you countless hours in the course of your career. If you have to do it because of broken 3rd party API. ... Well, all bets are off anyway.

[orafaaraujo]

Hey, guys

So If I have a NPE I can't get it with .catch { e -> emit(Result.Error(Exception(e))) }?
Tthis is what is happening here... I'm receiving an NPE and my catch is getting it, but it's not emitting my Result.Error . The app is still crashing 😞