On Sun, 16 Jun 2024 06:36:00 GMT, Jaikiran Pai <j...@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 incrementally with one additional > commit since the last revision: > > Chen's suggestion - improve code comment Hi Jai, thanks for picking this up. Several points of discussion here. Nothing fatal, but as I said in 2019 :-) just a few wrinkles to iron out. I looked mainly at Deflater, but I think the issues also all apply to Inflater too. 1) 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. 2) 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. 3) 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. 4) 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. This would only be a problem if a) some arrangement of TWR ends up calling close() twice, which apparently it doesn't; b) there is a subclass; and c) the subclass's end() method is not idempotent. As far as I can tell we haven't found any example of any of these. It thus seems to me that having extra logic in close() to avoid calling end() more than once is solving a non-problem. It also fits more closely to the model of "close() is simply a synonym for end()". ------------- PR Comment: https://git.openjdk.org/jdk/pull/19675#issuecomment-2172433871
