Skip to content

Conversation

@kbendick
Copy link
Contributor

@kbendick kbendick commented Jul 12, 2022

The runSafely function gives an error prone warning for throwing from a finally block.

Originally, we fixed this via suppression in #5190, but I noticed this function was unused and so chose to deprecate it instead in #5205.

@rdblue wanted to keep the function, as it's tested and could be useful in the future.

So this PR:

  • fixes the issue discovered in [API] Fix ErrorProne warnings in API module #5190 of throwing failure when it's null
  • attempts to throw failure after adding e as a suppressed error when it's non-null
  • adds the @Finally suppression to remove the error prone log from the build.

Warning that is logged on build:

iceberg/api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java:124: warning: [Finally] If you return or throw from a finally, then values returned or thrown from the try-catch block will be ignored. Consider using try-with-resources instead.
            throw new RuntimeException("Unknown exception in finally block", failure);
            ^
    (see https://errorprone.info/bugpattern/Finally)

} catch (Exception e) {
LOG.warn("Suppressing failure in finally block", e);
if (failure != null) {
LOG.warn("Suppressing failure in finally block", e);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved this log because we're not suppressing e in the other block.

@github-actions github-actions bot added the API label Jul 12, 2022
if (failure != null) {
LOG.warn("Suppressing failure in finally block", e);
failure.addSuppressed(e);
tryThrowAs(failure, e1Class);
Copy link
Contributor

@dimas-b dimas-b Jul 12, 2022

Choose a reason for hiding this comment

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

This is probably not necessary and actually goes against the ErrorProne guideline of not throwing from finally.

The only way failure can be non-null here is if it was thrown from the main try block, in which case the generated bytecode will ensure it is re-thrown when finally completes.

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, failure is already thrown because it is thrown in the catch block. tryThrowAs needs to be removed here. The logic was correct before, except for the wrong exception in the next block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL. Will remove. Thanks guys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh actually yeah I understand. The failure would have already been thrown, just the finally block gets run and we add one more suppression to it. So we don't need to throw it again. Thanks for the explanation.

@kbendick kbendick force-pushed the kb-update-run-safely-and-suppress-warning branch from b34169f to 0f7142c Compare July 12, 2022 21:34
@rdblue rdblue merged commit 774b2f7 into apache:master Jul 12, 2022
@rdblue
Copy link
Contributor

rdblue commented Jul 12, 2022

Thanks, @kbendick!

@kbendick kbendick deleted the kb-update-run-safely-and-suppress-warning branch July 13, 2022 06:27
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.

3 participants