feat(core): Add ExceptionUtils.handleFatal to rethrow non-recoverable throwables - #5907
feat(core): Add ExceptionUtils.handleFatal to rethrow non-recoverable throwables#5907markushi wants to merge 1 commit into
Conversation
|
📲 Install BuildsAndroid
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d15471f | 315.61 ms | 360.22 ms | 44.61 ms |
| bb0ff41 | 344.70 ms | 413.82 ms | 69.12 ms |
| b0aa73e | 320.00 ms | 371.82 ms | 51.82 ms |
| 8558cac | 306.16 ms | 355.24 ms | 49.09 ms |
| ae7fed0 | 293.84 ms | 380.22 ms | 86.38 ms |
| 5b66efd | 308.67 ms | 363.85 ms | 55.18 ms |
| ee747ae | 400.46 ms | 423.61 ms | 23.15 ms |
| 02e6bc8 | 396.57 ms | 469.08 ms | 72.51 ms |
| 11f90db | 314.26 ms | 372.43 ms | 58.17 ms |
| e63ad34 | 297.04 ms | 369.90 ms | 72.86 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d15471f | 1.58 MiB | 2.13 MiB | 559.54 KiB |
| bb0ff41 | 0 B | 0 B | 0 B |
| b0aa73e | 0 B | 0 B | 0 B |
| 8558cac | 0 B | 0 B | 0 B |
| ae7fed0 | 1.58 MiB | 2.12 MiB | 551.77 KiB |
| 5b66efd | 1.58 MiB | 2.13 MiB | 559.07 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| 02e6bc8 | 0 B | 0 B | 0 B |
| 11f90db | 0 B | 0 B | 0 B |
| e63ad34 | 0 B | 0 B | 0 B |
| if (throwable instanceof VirtualMachineError || throwable instanceof ThreadDeath) { | ||
| throw (Error) throwable; | ||
| } | ||
| if (throwable instanceof InterruptedException) { |
There was a problem hiding this comment.
I was a bit torn apart if this utility method should take care of interrupts as well, and decided to do it, as it will keep all call sites more tight. Along with that change the method is now a generic handleFatal instead of a rethrowFatal. That doesn't tell a lot about the method, but I guess it will be a common pattern everywhere. - Happy for any opinions on this!
There was a problem hiding this comment.
Seems reasonable to me, and definitely nice to have a common facility for restoring the interrupt flag.
The only potential concern I can think of are methods that declare throws InterruptedException and need to use handleFatal(), as sometimes clients might expect either an InterruptedException or an interrupted flag to be set, but not both. But those methods could simply re-throw the InterruptedException before calling handleFatal().
The alternative would be to have separate methods similar to what Project Reactor does:
- ExceptionUtils.rethrowIfJvmFatal()
- ExceptionUtils.restoreInterruptIfNeeded()
- ExceptionUtils.rethrowIfCancellation() (for coroutine CancellationException)
But I prefer the current all-in-one approach more 👍
There was a problem hiding this comment.
That's some good input! We could improve the ergonomics slightly by introducing method chaining. E.g.
ExceptionUtils.restoreInterrupt().rethrowIfJvmFatal()
Let me task the clanker to have a look at this and the actual impact on our codebase
There was a problem hiding this comment.
I think it depends on the codebase and if we have use cases where we would need to do one and not the other. Can you ask the clanker to give you a report if we have such cases?
0xadam-brown
left a comment
0xadam-brown
left a comment
There was a problem hiding this comment.
Nice – and thanks for this!
One renaming comment for your consideration, but no blockers 🥇
| * | ||
| * @param throwable - the throwable to check | ||
| */ | ||
| public static void handleFatal(final @NotNull Throwable throwable) { |
There was a problem hiding this comment.
l: Thoughts about rethrowIfFatal()?
To me, that'd make its purpose more obvious at the call site (and I'm fine with its cheating a bit w/r/t restoring the interrupted flag).
There was a problem hiding this comment.
Yeah sounds good to me too, let's cheat a little 😅
There was a problem hiding this comment.
Agreed with rethrowIfFatal!
| if (throwable instanceof VirtualMachineError || throwable instanceof ThreadDeath) { | ||
| throw (Error) throwable; | ||
| } | ||
| if (throwable instanceof InterruptedException) { |
There was a problem hiding this comment.
Seems reasonable to me, and definitely nice to have a common facility for restoring the interrupt flag.
The only potential concern I can think of are methods that declare throws InterruptedException and need to use handleFatal(), as sometimes clients might expect either an InterruptedException or an interrupted flag to be set, but not both. But those methods could simply re-throw the InterruptedException before calling handleFatal().
The alternative would be to have separate methods similar to what Project Reactor does:
- ExceptionUtils.rethrowIfJvmFatal()
- ExceptionUtils.restoreInterruptIfNeeded()
- ExceptionUtils.rethrowIfCancellation() (for coroutine CancellationException)
But I prefer the current all-in-one approach more 👍
| public static void handleFatal(final @NotNull Throwable throwable) { | ||
| // VirtualMachineError covers OutOfMemoryError, StackOverflowError, InternalError, and | ||
| // UnknownError | ||
| if (throwable instanceof VirtualMachineError || throwable instanceof ThreadDeath) { |
There was a problem hiding this comment.
(Agree that we should omit re-throwing LinkageError b/c of the compileOnly issue illustrated here.)
There was a problem hiding this comment.
That's a valid point but I think we should re-throw LinkageError and only catch it where we expect it to be thrown as you did there.
runningcode
left a comment
runningcode
left a comment
There was a problem hiding this comment.
Thanks for doing this! It looks good so far.
| * | ||
| * @param throwable - the throwable to check | ||
| */ | ||
| public static void handleFatal(final @NotNull Throwable throwable) { |
There was a problem hiding this comment.
Agreed with rethrowIfFatal!
| if (throwable instanceof VirtualMachineError || throwable instanceof ThreadDeath) { | ||
| throw (Error) throwable; | ||
| } | ||
| if (throwable instanceof InterruptedException) { |
There was a problem hiding this comment.
I think it depends on the codebase and if we have use cases where we would need to do one and not the other. Can you ask the clanker to give you a report if we have such cases?
3 participants
Summary
Adds
ExceptionUtils.handleFatal(Throwable), a small utility for use inside broadcatch (Throwable t)blocks. It rethrowsVirtualMachineError(e.g.OutOfMemoryError,StackOverflowError) andThreadDeathas-is, and restores the thread's interrupt flag forInterruptedExceptioninstead of swallowing it. All other throwables are left untouched for the caller to handle as before.Closes #5865
#skip-changelog