On Mon, 2 Dec 2024 05:26:30 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 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 22 additional > commits since the last revision: > > - provide guidance to subclasses on which method to override for cleaning up > resources > - Revert "Roger's suggestion - Make Inflater.close() and Deflater.close() > final, also update the new tests to match this change" > > This reverts commit b60181bbb4be9fac294b16820cd02017de71783e. > - merge latest from master branch > - update end() to remove mention of other methods throwing > IllegalStateException > - update the class level documentation of Inflater to match the updates in > Deflater > - merge latest from master branch > - improve Deflater class level doc > - Stuart's review - improve end() API doc > - merge latest from master branch > - missed a few methods for specifying IllegalStateException > - ... and 12 more: https://git.openjdk.org/jdk/compare/e0f0a904...42ff9059 src/java.base/share/classes/java/util/zip/Inflater.java line 58: > 56: * To release resources used by the {@code Inflater}, applications must > call the > 57: * {@link #end()} method. After {@code end()} has been called, subsequent > calls > 58: * to several methods of the {@code Inflater} will throw an {@link > IllegalStateException}. Since `close()` is not mentioned here, there may be some come confusion about whether `end()` still needs to be called within T-W-R. test/jdk/java/util/zip/DeflaterClose.java line 43: > 41: > 42: /** > 43: * Closes the Deflater multiple times and then expects close() and > end() to be called that Should this be "close() *or* end()"? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19675#discussion_r1866228297 PR Review Comment: https://git.openjdk.org/jdk/pull/19675#discussion_r1866231672
