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

[Roger Riggs]

On Mon, 2 Dec 2024 05:26:30 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 22 additional commits since the last revision:
> 
>  - provide guidance to subclasses on which method to override for cleaning up resources
>  - Revert "Roger's suggestion - Make Inflater.close() and Deflater.close() final, also update the new tests to match this change"
>    
>    This reverts commit b60181bbb4be9fac294b16820cd02017de71783e.
>  - merge latest from master branch
>  - update end() to remove mention of other methods throwing IllegalStateException
>  - update the class level documentation of Inflater to match the updates in Deflater
>  - merge latest from master branch
>  - improve Deflater class level doc
>  - Stuart's review - improve end() API doc
>  - merge latest from master branch
>  - missed a few methods for specifying IllegalStateException
>  - ... and 12 more: https://git.openjdk.org/jdk/compare/e0f0a904...42ff9059

src/java.base/share/classes/java/util/zip/Inflater.java line 58:

> 56:  * To release resources used by the {@code Inflater}, applications must call the
> 57:  * {@link #end()} method. After {@code end()} has been called, subsequent calls
> 58:  * to several methods of the {@code Inflater} will throw an {@link IllegalStateException}.

Since `close()` is not mentioned here, there may be some come confusion about whether `end()` still needs to be called within T-W-R.

test/jdk/java/util/zip/DeflaterClose.java line 43:

> 41: 
> 42:     /**
> 43:      * Closes the Deflater multiple times and then expects close() and end() to be called that

Should this be "close() *or* end()"?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19675#discussion_r1866228297
PR Review Comment: https://git.openjdk.org/jdk/pull/19675#discussion_r1866231672