On Thu, 26 Feb 2026 11:28:17 GMT, Jaikiran Pai <[email protected]> wrote:
> Can I please get a review of this change which addresses the issue noted in > https://bugs.openjdk.org/browse/JDK-8369181? > > As noted in that issue, if `finish()` is called on a `InflaterOutputStream` > that was constructed without passing a `Inflater`, then any subsequent > `write()`s on that `InflaterOutputStream` result in an > `IllegalStateException`, because we close the `Inflater` in `finish()`. The > commit in this PR, fixes the issue by throwing the specified `IOException` in > place of the `IllegalStateException`. > > At the same time, the documentation of `finish()` has been enhanced to > clarify the current behaviour, through a `@implSpec`. > > Alternative approaches of deprecating finish() and/or not closing the default > Inflater were considered, but given the current long standing implementation > of finish(), it was decided to merely specify the current behaviour of > closing the default Inflater in finish(). > > A new jtreg test has been added to reproduce the issue and verify the fix. > tier1, tier2, tier3 testing of this change completed without any related > issues. > > I'll file a CSR shortly for this change. I like this approach. I believe it's the best for keeping compatibility risks low. I was a bit uncertain about specifying calling `Inflater::close()` when the implementation actually call `Inflater::end()` but this probably don't matter since it's only called on the default inflater which is not user-supplied. src/java.base/share/classes/java/util/zip/InflaterOutputStream.java line 243: > 241: flush(); > 242: if (usesDefaultInflater) { > 243: inf.end(); close() and end() are the same thing for the default inflater so I guess this is fine and not violating the spec. src/java.base/share/classes/java/util/zip/InflaterOutputStream.java line 288: > 286: if (usesDefaultInflater && defaultInflaterClosed) { > 287: throw new IOException("Inflater closed"); > 288: } Since we're already checking for `closed` before checking for `null` I guess it makes sense to make this check here in second position. I wonder if it could/should be included in `ensureOpen()`? Maybe, or maybe not - I see `ensureOpen()` is called from `finish()` too. So if we moved that check to `ensureOpen()` then calling `finish()` twice in succession would throw. What you have now will probably keep compatibility risk at minima. ------------- PR Review: https://git.openjdk.org/jdk/pull/29935#pullrequestreview-3860485396 PR Review Comment: https://git.openjdk.org/jdk/pull/29935#discussion_r2858632959 PR Review Comment: https://git.openjdk.org/jdk/pull/29935#discussion_r2858648728
