On Fri, 5 Jun 2026 06:37:27 GMT, Jaikiran Pai <[email protected]> wrote:
> Can I please get a review of this change which proposes to address > https://bugs.openjdk.org/browse/JDK-8385924? > > For reasons explained in that issue, the `java.util.zip.GZIPInputStream` > class currently behaves differently depending on which version of Java is in > use. The change in this current PR proposes to introduce a system property > which allows applications to decide whether they prefer the decades old > behaviour of `GZIPInputStream` where it uses `InputStream.available()` for > deciding whether to read a subsequent member in the stream, or whether the > application prefers the more recent behaviour of `GZIPInputStream` where it > skips the `available()` call and instead directly does a read operation for > detecting a subsequent member. > > The system property by default isn't set which means that `GZIPInputStream` > will call `InputStream.available()` and thus matches its decades old > behaviour. The introduction of this system property is a stopgap measure as > noted in https://github.com/openjdk/jdk/pull/30925#discussion_r3318466104 : > >> For JDK 27, introduce a system property that controls the reliable handling >> of streams with more than one member. Long standing behavior does not handle >> the case where the underlying stream doesn't have a useful avaialble() >> method and it additionally swallows any exception when reading ahead. The >> system property can be documented as an implNote in the 2-arg constructor or >> class description. The property will need default to long standing behavior >> (not the more recent/changed behavior). > > As noted in that same comment, the long term solution involves enhancing the > `GZIPInputStream` class to introduce a constructor which allows applications > to decide how the `GZIPInputStream` should behave when dealing with multiple > members: > >> For JDK 28 or later, introduce a new constructor to create a GZIPInputStream >> that works like it should have done in JDK 1.1 when the API was originally >> introduced. We discussed two possible sets of parameters. We also discussed >> maybe deprecating the two existing constructors due to the issue of treating >> available==0 as EOF and the issue of swallowing exceptions. > > That new API will need careful thought and time and thus the current proposal > to introduce this system property in the meantime. > > A new jtreg test has been introduced to verify the usage of this system > property. A CSR will be proposed shortly. Existing tests and this new test > continue to pass with this change. > > --------- > - [x] I confirm that I make thi... Hi @jaikiran, Thanks for this PR. I think the implementation is fine. I just have some suggestions regarding the comments and would like to request to state the `@implNote` slightly more precisely. Best regards, Volker src/java.base/share/classes/java/util/zip/GZIPInputStream.java line 69: > 67: * is performed on the underlying stream for a subsequent member. By > default, > 68: * the {@code jdk.util.gzip.tryReadAheadAfterTrailer} system property is > unset, and > 69: * {@code InputStream.available()} gets called. This reads well, but I think we should add a sentence about the risk of using `InputStream.available()`. E.g.: ... * is performed on the underlying stream for a subsequent member. By default, * the {@code jdk.util.gzip.tryReadAheadAfterTrailer} system property is unset, and * {@code InputStream.available()} gets called which can lead to a silent drop of * concatenated GZIP members. src/java.base/share/classes/java/util/zip/GZIPInputStream.java line 351: > 349: // be present > 350: if (!readNextMember) { > 351: return true; // no more members exist I think this comment is misleading. It should either read `// no more members exist or alwaysReadNextMember was false and available() returned 0` (but that just repeating the previous expression) or be removed. src/java.base/share/classes/java/util/zip/GZIPInputStream.java line 367: > 365: if (numRemainingInInflater > m) { > 366: // position the inflater's input buffer to the start of next > member's deflated > 367: // data You already have longer lines in this file (e.g. line 362 above) so no need to put data on a new line. ------------- Changes requested by simonis (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/31396#pullrequestreview-4450875250 PR Review Comment: https://git.openjdk.org/jdk/pull/31396#discussion_r3374481984 PR Review Comment: https://git.openjdk.org/jdk/pull/31396#discussion_r3374363249 PR Review Comment: https://git.openjdk.org/jdk/pull/31396#discussion_r3374405548
