RFR: 8225763: Inflater and Deflater should implement AutoCloseable

[Chen Liang]

On Wed, 12 Jun 2024 10:45: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.

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

> 902:     public void close() {
> 903:         synchronized (zsRef) {
> 904:             // check if already closed

Should we comment `// in case subclasses override the closed check in end()` instead? Otherwise the duplicate comments and checks are confusing.

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

> 722:     public void close() {
> 723:         synchronized (zsRef) {
> 724:             // check if already closed

Same remark, we should mention subclass override close risk

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

> 1: /*
> 2:  * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.

Suggestion:

 * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.

Same for `InflaterClose.java`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19675#discussion_r1637091530
PR Review Comment: https://git.openjdk.org/jdk/pull/19675#discussion_r1637092387
PR Review Comment: https://git.openjdk.org/jdk/pull/19675#discussion_r1637092809