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

[Lance Andersen]

On Fri, 15 Nov 2024 11:19:43 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> src/java.base/share/classes/java/util/zip/Deflater.java line 66:
>> 
>>> 64:  * usage with try-with-resources, this class implements {@link AutoCloseable}. The
>>> 65:  * {@link #close()} method of this class calls {@code end()} to clean up its
>>> 66:  * resources. Subclasses are responsible for the cleanup of resources
>> 
>> As part of the clarification, do we need to state the verbiage above regarding try-with-resources specifically?
>> 
>> I did a quick scan of some of the other classes implementing AutoClosable and did not see many cases of that in the documentation most likely due to that behavior is clear in the documentation for AutoClosable
>
> 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.

>> src/java.base/share/classes/java/util/zip/Deflater.java line 878:
>> 
>>> 876:      * If this method is invoked multiple times, the subsequent calls are treated as a no-op.
>>> 877:      * Several other methods defined by this class will throw an {@link IllegalStateException}
>>> 878:      * if invoked on a closed {@code Deflater}.
>> 
>> All of the methods which call ensureOpen should either document that an IllegalStateException may be thrown or you could add it to the header of the class.
>
> 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.

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

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