Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OAK-11571: commons: add Closer class (similar to Guava Closer) #2181

Open
wants to merge 16 commits into
base: trunk
Choose a base branch
from

Conversation

reschke
Copy link
Contributor

@reschke reschke commented Mar 13, 2025

Note

1b60437

which added tests for Guava Closer. The Oak implementation only was written after tests were there.

@reschke reschke self-assigned this Mar 13, 2025
Copy link

github-actions bot commented Mar 13, 2025

Commit-Check ✔️

@reschke reschke marked this pull request as draft March 13, 2025 15:38
@reschke reschke marked this pull request as ready for review March 13, 2025 15:40
@reschke
Copy link
Contributor Author

reschke commented Mar 14, 2025

I believe all comments were based on misunderstandings about what Guava does; see again d254e8f which has the same tests running with Guava.

@thomasmueller - every time we instantiate a collection with a non-default size, readers of the code will ask themselves: why? and does it really matter here?

@reschke
Copy link
Contributor Author

reschke commented Mar 14, 2025

Things IMHO left to do:

  • address some Sonar warnings
  • improve Javadoc
  • rename the completely confusingly named method rethrow() to rethrowOnClose() - not done, because I misunterstood the semantics
  • bikeshed about the package name - any opinions here???

Note that once this is in (and used throughout); we can get rid of the entire Guava common.io package.

toThrow = exception;
}
}
}
Copy link
Contributor

@mbaedke mbaedke Mar 14, 2025

Choose a reason for hiding this comment

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

What about RuntimeExceptions (or any Throwable that is not an IOException)? If they are thrown from closeable.close(), they will not be catched, even if suppressExceptionsOnClose is true (see modified test in comment below).

fail("should not throw but got: " + e);
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

    public void compareClosers() {
        // when rethrow was called, Exceptions that happened upon close will be swallowed

        com.google.common.io.Closer guavaCloser = com.google.common.io.Closer.create();
        Closer oakCloser = Closer.create();

        try {
            throw oakCloser.rethrow(new InterruptedException());
        } catch (Exception e) {}

        try {
            throw guavaCloser.rethrow(new InterruptedException());
        } catch (Exception e) {}

        try {
            oakCloser.register(() -> { throw new RuntimeException(); });
            oakCloser.close();
        } catch (Exception e) {
            fail("should not throw but got: " + e);
        }

        try {
            guavaCloser.register(() -> { throw new RuntimeException(); });
            guavaCloser.close();
        } catch (Exception e) {
            fail("should not throw but got: " + e);
        }
    }

would currently fail.

Copy link
Contributor

@mbaedke mbaedke left a comment

Choose a reason for hiding this comment

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

RuntimeException are not yet treated correctly, see inline comments.

…e unchecked exceptions thrown by Closeables when rethrow was called
@reschke reschke requested a review from mbaedke March 14, 2025 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants