On Fri, 15 Dec 2023 03:20:01 GMT, Archie Cobbs <aco...@openjdk.org> wrote:
>> `GZIPInputStream`, when looking for a concatenated stream, relies on what
>> the underlying `InputStream` says is how many bytes are `available()`. But
>> this is inappropriate because `InputStream.available()` is just an estimate
>> and is allowed (for example) to always return zero.
>>
>> The fix is to ignore what's `available()` and just proceed and see what
>> happens. If fewer bytes are available than required, the attempt to extend
>> to another stream is canceled just as it was before, e.g., when the next
>> stream header couldn't be read.
>
> Archie Cobbs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address review comments.
A few minor suggestions.
test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 52:
> 50: buf.reset();
> 51: for(int i = 0; i < 100; i++)
> 52: buf.write(gz);
Whitespace after `for`, braces are recommended even when optional in the
language:
Suggestion:
for (int i = 0; i < 100; i++) {
buf.write(gz);
}
test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 53:
> 51: for(int i = 0; i < 100; i++)
> 52: buf.write(gz);
> 53: final byte[] gz32 = buf.toByteArray();
Drop final, consider finding a more expressive name:
Suggestion:
byte[] concatenatedGz = buf.toByteArray();
test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 56:
> 54:
> 55: // (a) Read it back from a stream where available() is accurate
> 56: long count1 = countBytes(new GZIPInputStream(new
> ByteArrayInputStream(gz32)));
This is the expected number of bytes to be read. This could be calculated
directly from the uncompressed data. At least the name should express that this
is the expected number of bytes:
Suggestion:
long expectedBytes = countBytes(new GZIPInputStream(new
ByteArrayInputStream(gz32)));
test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 62:
> 60:
> 61: // They should be the same
> 62: Assert.assertEquals(count2, count1);
This could be a static import. And for the assertion failure message to be
correct, the expected value must come first:
Suggestion:
assertEquals(expectedBytes, actualBytes);
An alternative here could be to store the original uncompressed data and
compare that to the full inflated data read from GZIPInputStream using
assertArrayEquals. The length alone is a bit of a weak proxy for equality.
-------------
PR Review: https://git.openjdk.org/jdk/pull/17113#pullrequestreview-1784136773
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428015571
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428017078
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428019017
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428020568
