On Tue, 26 Oct 2021 06:30:39 GMT, Ravi Reddy <rre...@openjdk.org> wrote:
>> Hi all,
>>
>> Please review this fix for Infinite loop in ZipOutputStream.close().
>> The main issue here is when ever there is an exception during close
>> operations on GZip we are not setting the deflator to a finished state which
>> is leading to an infinite loop when we try writing on the same GZip
>> instance( since we use while(!def.finished()) inside the write operation).
>>
>> Thanks,
>> Ravi
>
> Ravi Reddy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8193682 : Infinite loop in ZipOutputStream.close()
I think overall this looks good. Thank you for continuing to work on this.
I do believe it would be worth adding a CSR just to document the behavior
realizing that it was always left undefined in the past
Please also add a comment to each test describing its intent
test/jdk/java/util/zip/GZIP/GZipLoopTest.java line 41:
> 39: public class GZipLoopTest {
> 40: private static final int FINISH_NUM = 512;
> 41: private static OutputStream outStream = new OutputStream() {
Please add a comment describing the intent of outstream and for FINISH_NUM.
You might also consider a different name vs FINISH_NUM. Perhaps the comment
will clarify this
test/jdk/java/util/zip/GZIP/GZipLoopTest.java line 57:
> 55: @Test
> 56: public void testGZipClose() throws IOException {
> 57: rand.nextBytes(b);
You could possibly consider using a BeforeTest or BeforeMethod if you choose to
reduce redundancy. No biggie otherwise.
test/jdk/java/util/zip/GZIP/GZipLoopTest.java line 65:
> 63: } catch (IOException e) {
> 64: //expected
> 65: }
For the above if an IOException is expected, should this be tested via
assertThrows()?
-------------
PR: https://git.openjdk.java.net/jdk/pull/5522
