On Fri, 15 Dec 2023 16:30: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 second round of review comments.
The test is shaping up nicely. Since it's a new test it should use JUnit 5.
Also included a couple suggestions, I'll stop now, promise! :)
test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 30:
> 28: */
> 29:
> 30: import org.junit.Test;
Let's use JUnit 5:
Suggestion:
import org.junit.jupiter.api.Test;
test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 36:
> 34: import java.util.zip.*;
> 35:
> 36: import static org.junit.Assert.assertArrayEquals;
Let's use JUnit 5:
Suggestion:
import static org.junit.jupiter.api.Assertions.assertArrayEquals;
test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 53:
> 51: byte[] compressedN = repeat(compressed1, NUM_COPIES);
> 52:
> 53: // (a) Read back copied compressed data from a stream where
> available() is accurate and verify
Suggestion:
// (a) Read back inflated data from a stream where available() is
accurate and verify
test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 54:
> 52:
> 53: // (a) Read back copied compressed data from a stream where
> available() is accurate and verify
> 54: byte[] readback1 = new GZIPInputStream(new
> ByteArrayInputStream(compressedN)).readAllBytes();
These readback lines got rather long. Perhaps consider extracting the GZIP
reading into a method taking the source InputStream as a parameter?
Suggestion:
byte[] readback1 = inflateFrom(new ByteArrayInputStream(compressedN));
test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 57:
> 55: assertArrayEquals(uncompressedN, readback1);
> 56:
> 57: // (b) Read back copied compressed data from a stream where
> available() always returns zero and verify
Suggestion:
// (b) Read back inflated data from a stream where available() always
returns zero and verify
test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 58:
> 56:
> 57: // (b) Read back copied compressed data from a stream where
> available() always returns zero and verify
> 58: byte[] readback2 = new GZIPInputStream(new
> ZeroAvailableStream(new ByteArrayInputStream(compressedN))).readAllBytes();
Suggestion:
byte[] readback2 = inflateFrom(new ZeroAvailableStream(new
ByteArrayInputStream(compressedN)));
-------------
PR Review: https://git.openjdk.org/jdk/pull/17113#pullrequestreview-1784857282
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428434843
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428435461
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428452384
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428455030
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428452815
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428456971
