Skip to content

Add ExceptionUtil.runSafely#1909

Merged
rdblue merged 5 commits intoapache:masterfrom
rdblue:add-run-safely
Dec 17, 2020
Merged

Add ExceptionUtil.runSafely#1909
rdblue merged 5 commits intoapache:masterfrom
rdblue:add-run-safely

Conversation

@rdblue
Copy link
Contributor

@rdblue rdblue commented Dec 11, 2020

This adds a utility method that accepts blocks as lambdas and correctly handles exceptions. Catch and finally blocks are always run and any exceptions thrown in those blocks are added to the main block's exception as suppressed exceptions. If the main block does not throw an exception, but finally does then the finally exception is thrown.

This currently supports up to 3 checked exception classes.

@github-actions github-actions bot added the core label Dec 11, 2020
tryThrowAs(failure, e2Class);
tryThrowAs(failure, e3Class);
tryThrowAs(failure, RuntimeException.class);
throw new RuntimeException("Unknown throwable", failure);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we get here? Is this just for safety? From what I can tell we only allow the block to have called exceptions for e1 -> e3, and all that's left should be RuntimeExceptions right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally, this block caught Throwable, but I changed it to Exception thinking that we should not catch Error and run the catch block. In that case, this line was just for safety.

But I've changed it back to Throwable after looking at this comment. I thought more about it and this block does need to run if Error is thrown instead of Exception. If Error is thrown but not caught here, then the finally block will run without the failure set and would throw any exception from that block instead of suppressing it.

I think if we want to do no work when Error is thrown, then this should specifically check for Error, but I think that it is correct to run everything on Error because that's the superclass of things like AssertionError. If an assert fails, we still want to run everything like normal.

Copy link
Contributor

@aokolnychyi aokolnychyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nothing to add to the existing comments. Thanks, @rdblue.

@rdblue rdblue merged commit e532f11 into apache:master Dec 17, 2020
@rdblue
Copy link
Contributor Author

rdblue commented Dec 17, 2020

Tests are passing, so I'll merge this. Thanks @yyanyy, @RussellSpitzer, and @aokolnychyi for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants