On Fri, 15 Nov 2024 12:17:47 GMT, Lance Andersen <lan...@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
