RFR: 8225763: Inflater and Deflater should implement AutoCloseable [v5]

[Lance Andersen]

On Thu, 14 Nov 2024 04:14:03 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Can I please get a review of this enhancement which proposes to enhance `java.util.zip.Deflater/Inflater` classes to now implement `AutoCloseable`?
>> 
>> The actual work for this was done a few years back when we discussed the proposed approaches and then I raised a RFR. At that time I couldn't take this to completion. The current changes in this PR involve the implementation that was discussed at that time and also have implemented the review suggestions from that time. Here are those previous discussions and reviews:
>> 
>> https://mail.openjdk.org/pipermail/core-libs-dev/2019-June/061079.html
>> https://mail.openjdk.org/pipermail/core-libs-dev/2019-July/061177.html
>> https://mail.openjdk.org/pipermail/core-libs-dev/2019-July/061229.html
>> 
>> To summarize those discussions, we had concluded that:
>> - `Deflater` and `Inflater` will implement the `AutoCloseable` interface
>> -  In the `close()` implementation we will invoke the `end()` method (`end()` can be potentially overridden by subclasses).
>> - `close()` will be specified and implemented to be idempotent. Calling `close()` a second time or more will be a no-op.
>> - Calling `end()` and then `close()`, although uncommon, will also support idempotency and that `close()` call will be a no-op.
>> - However, calling `close()` and then `end()` will not guarantee idempotency and depending on the implementing subclass, the `end()` may throw an exception.
>> 
>> New tests have been included as part of these changes and they continue to pass along with existing tests in tier1, tier2 and tier3. When I had originally added these new tests, I hadn't used junit. I can convert them to junit if that's preferable.
>> 
>> I'll file a CSR shortly.
>
> Jaikiran Pai has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision:
> 
>  - merge latest from master branch
>  - update tests to match the new specification
>  - Stuart's review - update the close() and end() expectations
>  - Stuart's review - improve class level javadoc
>  - merge latest from master branch
>  - merge latest from master branch
>  - Chen's suggestion - improve code comment
>  - convert the tests to junit
>  - fix whitespace
>  - 8225763: Inflater and Deflater should implement AutoCloseable

HI Jai,

Thanks for tackling this and sticking with it.  I have few comments below which could be considered for both Deflater/Inflater

Also, extra credit for:

-  converting the CloseinflaterDeflaterTest to use junit from testNG
- converting TotalInOut to use junit from the older test structure

src/java.base/share/classes/java/util/zip/Deflater.java line 66:

> 64:  * usage with try-with-resources, this class implements {@link AutoCloseable}. The
> 65:  * {@link #close()} method of this class calls {@code end()} to clean up its
> 66:  * resources. Subclasses are responsible for the cleanup of resources

As part of the clarification, do we need to state the verbiage above regarding try-with-resources specifically?

I did a quick scan of some of the other classes implementing AutoClosable and did not see many cases of that in the documentation most likely due to that behavior is clear in the documentation for AutoClosable

src/java.base/share/classes/java/util/zip/Deflater.java line 878:

> 876:      * If this method is invoked multiple times, the subsequent calls are treated as a no-op.
> 877:      * Several other methods defined by this class will throw an {@link IllegalStateException}
> 878:      * if invoked on a closed {@code Deflater}.

All of the methods which call ensureOpen should either document that an IllegalStateException may be thrown or you could add it to the header of the class.

-------------

PR Review: https://git.openjdk.org/jdk/pull/19675#pullrequestreview-2436771806
PR Review Comment: https://git.openjdk.org/jdk/pull/19675#discussion_r1842632446
PR Review Comment: https://git.openjdk.org/jdk/pull/19675#discussion_r1842638400