RFR: 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream [v3]

[Eirik Bjorsnos]

On Fri, 15 Dec 2023 16:30:01 GMT, Archie Cobbs <acobbs at 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