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

[Stuart Marks]

On Fri, 15 Nov 2024 12:17:47 GMT, Lance Andersen <lancea at openjdk.org> wrote:

>> Stuart in one of his review comments in this PR had noted that:
>> 
>>> 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.
>> 
>> The current wording of that class level javadoc is my attempt to implement that suggestion. Do you think we can word it differently and yet convey the purpose of close()?
>
> Hi Jai,
> 
> I am not convinced we need to call this out specifically at the class level given  the classes implements AutoClosable and close indicates it calls end.  We will also  want to add a Release Note which can help further clarify this
> 
> Now that being said, what I would suggest instead is a simple code example in the class header which using try-with-resources would be more effective/helpful.

Deflater and Inflater are unusual in that their resources are released by calling `end()` instead of `close()`. So I think some mention of the situation is in order.

The first responsibility is for normative text (before the example) to impose a requirement on applications that they call `end()` when they are finished. Something like:

> To release resources used by this Deflater, applications must call the `end()` method.

This is probably a good place to add a statement about, after `end()` has been called, all subsequent operations will throw IllegalStateException.

I don't think we need to include the text about subclasses. I tried drafting something but it seemed clumsy and unnecessary. But I'm open to other opinions about whether such text should be included.

At this point I think it's ok to introduce an `@apiNote` that mentions that a `close()` method simply calls `end()`, and that this class implements `AutoCloseable` in order to facilitate use with try-with-resources. And finally include the big example snippet, which is modified to use try-with-resources.

In addition, wording below about finalization and Cleaner should be removed. The requirement to call `end()` is sufficient, and I don't think there's anything special about this class that merits a discussion about GC-driven cleanup.

>> Hello Lance, you are right - I forgot to do that when I changed it to throw IllegalStateException from the previous NullPointerException. I've now updated the PR to document this on individual methods in the Inflater/Deflater which throw this exception.
>
> Thank you.  It was an oversight that the original javadoc did not call out the NPE, but now is our chance to get it right with a more reasonable exception.

Overall I agree with defining that IllegalStateException is thrown after the Deflater is closed, and adding documentation of this to all the methods that throw it.

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

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