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

[Jaikiran Pai]

On Thu, 7 Nov 2024 14:16:36 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 nine additional commits since the last revision:
> 
>  - 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

Thank you Stuart for your inputs.

> class specification
>
> I think the class specification needs to be clearer about the positioning of end() and close(). The end() method has done the real work of "closing" since the classes were introduced. We're retrofitting them to to have a close() method that's essentially just a synonym for end(), and it's still perfectly legal for clients and subclasses to call end() instead of close(). I think we need to say that close() is present mainly to support AutoCloseable and try-with-resources.

I've now updated the class level javadoc of both Inflater and Deflater to be more explicit on the purpose of close() method and how it relates to the end() method.


> end() specification
>
> I don't think that "may throw an exception" is strong enough. Also, on these classes (not subclasses) end() is idemopotent, isn't it? If so, this should be specified. The end() specs need to say something like, closes and releases resources, etc., henceforth operations will throw an exception (which? -- see below) but that subsequent calls to end() do nothing.

I've updated both the end() and close() method javadocs. It now states that end() is idempotent and at the same time states that calling several other methods on a closed Inflater/Deflater will throw a specific exception (noted below). I copied over the sentence which states "Several other methods defined..." from an existing Java SE API.


> Which exceptions on closed Deflater/Inflater?
>
> Oddly, it doesn't seem to be specified anywhere what exceptions are thrown if operations are attempted on a closed object. And the implementation actually throws NPE??!? Ideally we should specify what exception is thrown in this circumstance, but NPE seems wrong. Maybe we want to change the behavior to throw a different exception type. IllegalStateException seems like a reasonable candidate.

Yes, it is indeed odd that it's throwing a `NullPointerException` when ensuring the Inflater/Deflater is open. I have changed that part to throw an `IllegalStateException` and make a mention of this exception in the end() method specification. Clearly, changing `NullPointerException` to `IllegalStateException` is a change in behaviour of public APIs and ideally should be done separately. But, given the context that it gets thrown only from an instance that's already closed (and thus of no real use), I think the chances of this change severly impacting applications is very limited. So, in my opinion, it's perhaps OK to include this change of exception type as part of this change and have it recorded through the same CSR that we use for this current enhancement. Do let me know if we should do that change separately.

> close() specification
>
> The 2019 discussion kind of assumed that we want close() to be idempotent. I'm not sure this is necessary. Alan said the implementation might need to be "heroic" but Peter Levart's arrangement (carried through here, apparently) isn't terribly bad. And @liach suggests more comments on this code, quite reasonably... but the comment begs the question, why do we need to avoid calling end() more than once? I'm rethinking the need for close() to be idempotent. Maybe close() should simply call end(). It would simplify the implementation and the specs -- the implSpec would simply say that it calls the end() method.

I ran various corpus experiments. Based on that what I have noticed is that, in that corpus, there are extremely few (less than 10) classes which override end() of either the Inflater or Deflater class. Furthermore, all these implementations have implemented their end() in a manner that calling it more than once will act as a no-op for subsequent invocations. So I think, like you note, we should just specify that close() calls end() and stop trying to prevent end() being called more than once. Given this, I've now updated the PR to do just that and also updated the new tests to match this specification updates.

tier testing is currently in progress for this current state of the PR. If the current state of the PR looks reasonable, then I'll focus on the CSR text for these changes.

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

PR Comment: https://git.openjdk.org/jdk/pull/19675#issuecomment-2462388705