Proposal: JavaUtils.throwUnchecked()

[Andrew Grieve]

Proposed CL: https://chromium-review.googlesource.com/c/chromium/src/+/5594290

Feel free to comment here or on the change.

Wrapping with RuntimeException

It is common to wrap a checked exception with a RuntimeException for cases where a checked exception is fatal, or not actually possible. In order to reduce the number of stack trace “caused by” clauses, and to save on binary size, use JavaUtils.throwUnchecked() instead.

try {
    somethingThatThrowsIOException();
} catch (IOException e) {
    // Bad - RuntimeException adds no context and creates longer stack traces.
    throw new RuntimeException(e);
    // Good - Original exception is preserved.
    throw JavaUtils.throwUnchecked(e);
}

[Nate Fischer]

Seems reasonable to me. I'm in favor of the change.

Nate Fischer | Software Engineer | ntf...@google.com

--
You received this message because you are subscribed to the Google Groups "java" group.
To unsubscribe from this group and stop receiving emails from it, send an email to java+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/CABiQX1UjaFEjbzCYCwt-TXLZEcEiL_gVBtFu7kxGCdLBfB6hJA%40mail.gmail.com.

[Tomasz Śniatowski]

Does this break (as in, require fixes in) call sites that do "catch (RuntimeException e) { ... }" when calling such methods?

--

Tomasz

[Andrew Grieve]

Yes, it would. And it does look like we have quite a few of these, which from my spot-checking seem to be placed around OS calls, although the odds that that doesn't apply always.

Unless you are going to rethrow the exception (which none of the ones I checked did), I don't think there is any benefit of catching RuntimeException over Exception...

That said, the intention is to use JavaUtils.throwUnchecked(e) in places where the exception will not be caught. Let's maybe add:

"Do not use this for places where the exception would plausibly be caught."

[Andrew Grieve]

Okay, updated the text. Thanks for that point Tomasz!

I've added a "quick primer", and a note about not catching RuntimeException.

Exceptions

A quick primer:

• Throwable: Base class for all exceptions
  • Error: Base class for exceptions which are meant to crash the app.
  • Exception: Base class for exceptions that make sense the catch.
    • RuntimeException: Base class for exceptions that do not need to be declared as throws (“unchecked exceptions”).

Broad Catch Handlers

Use catch statements that do not catch exceptions they are not meant to.

• There is rarely a valid reason to catch (Throwable t), since that includes the (generally unrecoverable) Error types.

Use catch (Exception e) when working with OS APIs that might throw (assuming the program can recover from them).

• There have been many cases of crashes caused by IllegalStateException / IllegalArgumentException / SecurityException being thrown where only RemoteException was being caught. If the program can recover from failed binder calls,

Do not use catch (RuntimeException e).

• It is useful to extend RuntimeException to make unchecked exception types, but the type does not make much sense in catch clauses, as there are not times when you'd want to catch all unchecked exceptions, but not also want to catch all checked exceptions.

Exception Messages

Avoid adding messages to exceptions that do not aid in debugging. For example:

try {
    somethingThatThrowsIOException();
} catch (IOException e) {

    // Bad - message does not tell you more than the stack trace does:
    throw new RuntimeException("Failed to parse a file.", e);
    // Good - conveys that this block failed along with the "caused by" exception.
    throw new RuntimeException(e);
    // Good - adds useful information.
    throw new RuntimeException(String.format("Failed to parse %s", fileName), e);
}

Wrapping with RuntimeException

It is common to wrap a checked exception with a RuntimeException for cases where a checked exception is not recoverable, or not possible. In order to reduce the number of stack trace “caused by” clauses, and to save on binary size, use JavaUtils.throwUnchecked() instead.

try {
    somethingThatThrowsIOException();
} catch (IOException e) {
    // Bad - RuntimeException adds no context and creates longer stack traces.
    throw new RuntimeException(e);
    // Good - Original exception is preserved.
    throw JavaUtils.throwUnchecked(e);
}

Do not use throwUnchecked() when the exception may want to be caught.

[Andrew Grieve]

I've submitted the change.