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

[Chen Liang]

On Thu, 13 Jun 2024 14:09:16 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> 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.
>
> Hello Chen, we want the close() to be idempotent irrespective of whether or not end() is overridden. That check in close() and the code comment on that line is to indicate that. If the comment is adding to confusion, I can remove it - I don't think it's a must to have it to understand the check.

I mean, we have updated the default implementation of end() to be idempotent. It now just appears confusing to readers that close() **seems to** perform redundant checks for idempotence until they realize subclasses can override end() to make it non-idempotent.

My suggestion is that our comment should say why we have this check here even though there's already an identical check in end().

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

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