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

[Alan Bateman]

On Wed, 8 Jan 2025 06:01:55 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 incrementally with one additional commit since the last revision:
> 
>   fix javadoc tag ordering - "@throws" after "@return"

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

> 50:  * <p>
> 51:  * This class deflates sequences of bytes into ZLIB compressed data format.
> 52:  * The input byte sequence is provided in either byte array or {@link ByteBuffer},

We should probably fix this sentence to say "in either a byte array or ...".

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

> 58:  * {@code Deflater} by calling either the {@link #end()} or the {@link #close()} method.
> 59:  * After the {@code Deflater} has been closed, subsequent calls to several methods
> 60:  * of the {@code Deflater} will throw an {@link IllegalStateException}.

This paragraph uses the definite article but there isn't a specific Deflater to speak of, and it's not a singleton. The first sentence of this paragraph might be better if re-worded "To release the resources used a Deflater, an application must close it by invoking its end() or close() method".

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

> 64:  * {@code try}-with-resources statement. The {@linkplain Deflater#close() close() method} simply
> 65:  * calls {@code end()}. Subclasses should override {@linkplain #end()} to clean up the
> 66:  * resources acquired by the subclass.

I wonder if the note for subclasses should move to the end method as implSpec, it looks out of place in the class API note.

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

> 890:      * and discards any unprocessed input.
> 891:      * <p>
> 892:      * If this method is invoked multiple times, the second and subsequent calls do nothing.

I think this could be clearer if you replace this sentence with "If the Deflater is already closed then invoking this method has no effect."

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19675#discussion_r1914314823
PR Review Comment: https://git.openjdk.org/jdk/pull/19675#discussion_r1914408496
PR Review Comment: https://git.openjdk.org/jdk/pull/19675#discussion_r1914413749
PR Review Comment: https://git.openjdk.org/jdk/pull/19675#discussion_r1914410532