-
Notifications
You must be signed in to change notification settings - Fork 78
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
Use AutoCloseable in some places where we previously used SafeCloseable, and standardize some SafeCloseable utilities #5274
Conversation
…le, and standardize some SafeCloseable utilities
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy with this change as-is, as it moves us in a good direction.
There is a case to be made that, when idempotent, we should prefer implementing Closeable
as well.
public class MyIdempotentIterator implements SafeCloseable, Closeable {
We could make a case for:
public interface SafeIdempotentCloseable extends SafeCloseable, Closeable {
to "simplify" the implementations a bit, and a place to add explicit javadoc (of course, maybe we should add more explicit javadoc on SafeCloseable regardless). That said, I don't think it would ever be appropriate to use this interface as an argument type.
// bad
public static void foo(SafeIdempotentCloseable x) { ... }
// good
public static <X extends SafeCloseable, Closeable> void bar(X x) { ... }
* resources that must be released. Such iterators must document this need, and appropriate measures (e.g. a | ||
* try-with-resources block) should be taken to ensure that they are {@link #close() closed}. Methods that return | ||
* streams over CloseableIterator instances should ensure that closing the resulting {@link Stream}, | ||
* {@link java.util.stream.IntStream IntStream}, {@link java.util.stream.LongStream LongStream}, or | ||
* {@link java.util.stream.DoubleStream DoubleStream} will also close the iterator. | ||
*/ | ||
public interface CloseableIterator<TYPE> extends Iterator<TYPE>, SafeCloseable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want stricter guarantee of idempotentcy w/ Closeable
? Not a blocker. Happy with any answer: yes, no, or yes, but later.
|
||
/** | ||
* Iteration support for values supplied by a column. | ||
* | ||
* @apiNote ColumnIterators must be explicitly {@link #close() closed} or used until exhausted in order to avoid | ||
* resource leaks. | ||
*/ | ||
public interface ColumnIterator<DATA_TYPE> extends CloseableIterator<DATA_TYPE> { | ||
public interface ColumnIterator<DATA_TYPE> extends CloseableIterator<DATA_TYPE>, SafeCloseable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same Q; if we decide that CloseableIterator
is not idempotent, we could still make that guarantee here if we wanted to.
try { | ||
SafeCloseable.closeAll(list.iterator()); | ||
} finally { | ||
list.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we extend Closeable
, given this is a form of idempotency?
In some discussions with @devinrsmith, we realized we could make some of the
SafeCloseable
code better by allowingAutoCloseable
usage in many existing patterns, at no loss of correctness or utility.