Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v14]
On Wed, 1 Dec 2021 05:35:11 GMT, Ravi Reddy 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: > > Change in test case name from GZipLoopTest.java to CloseDeflaterTest , > moved testcase to util/zip/ Marked as reviewed by coffeys (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v14]
> 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: Change in test case name from GZipLoopTest.java to CloseDeflaterTest , moved testcase to util/zip/ - Changes: - all: https://git.openjdk.java.net/jdk/pull/5522/files - new: https://git.openjdk.java.net/jdk/pull/5522/files/47697f96..6541de46 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5522=13 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5522=12-13 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5522.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5522/head:pull/5522 PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v13]
On Thu, 18 Nov 2021 19:09:18 GMT, Ravi Reddy 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: > > Added ZipException case to handle failure CopyZipFile.java - Only close the > deflater incase of IOException not in ZipException case scenario could you move the test up one directory to java/util/zip ? I don't think it's particular to GZIP any longer. Also perhaps a rename to something like CloseDeflaterTest etc. - PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v13]
On Thu, 18 Nov 2021 19:09:18 GMT, Ravi Reddy 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: > > Added ZipException case to handle failure CopyZipFile.java - Only close the > deflater incase of IOException not in ZipException case scenario Hi All, In the latest commit, I have resolved an issue in the case of ZipException in ZipOutputStream.closeEntry(). Now close the deflater in ZipOutputStream.closeEntry() only in case of an IOException and not with ZipException. Mach5 is green : https://mach5.us.oracle.com/mdash/jobs?search=id:rreddy-jdk-2028-1708-26358737 Thanks, Ravi - PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v13]
> 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: Added ZipException case to handle failure CopyZipFile.java - Only close the deflater incase of IOException not in ZipException case scenario - Changes: - all: https://git.openjdk.java.net/jdk/pull/5522/files - new: https://git.openjdk.java.net/jdk/pull/5522/files/d9112977..47697f96 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5522=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5522=11-12 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5522.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5522/head:pull/5522 PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v12]
On Wed, 17 Nov 2021 21:25:16 GMT, Lance Andersen wrote: > Hi Ravi, > > The current revision looks good to me. > > Best Lance I agree, commit d9112977 looks much better. There will be follow-up clarification needed to javadoc but they can be down with another JBS issue/PR. - PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v12]
On Wed, 17 Nov 2021 21:04:20 GMT, Ravi Reddy 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: > > Change in description of test case Hi Ravi, The current revision looks good to me. Best Lance - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v12]
> 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: Change in description of test case - Changes: - all: https://git.openjdk.java.net/jdk/pull/5522/files - new: https://git.openjdk.java.net/jdk/pull/5522/files/12a8371f..d9112977 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5522=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5522=10-11 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/5522.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5522/head:pull/5522 PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v11]
> 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: Added better try/catch/finally block at GZIPOutputStream.finish() , ZipOutputStream.closeEntry() and DeflaterOutputStream.close() - Changes: - all: https://git.openjdk.java.net/jdk/pull/5522/files - new: https://git.openjdk.java.net/jdk/pull/5522/files/c66535fc..12a8371f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5522=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5522=09-10 Stats: 31 lines in 3 files changed: 9 ins; 12 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/5522.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5522/head:pull/5522 PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v10]
> 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision: - Merge branch 'openjdk:master' into JDK-8193682 - 8193682: Infinite loop in ZipOutputStream.close() - 8193682 : Infinite loop in ZipOutputStream.close() - 8193682 : Infinite loop in ZipOutputStream.close() - 8193682 : Infinite loop in ZipOutputStream.close() - 8193682 : Infinite loop in ZipOutputStream.close() - 8193682 : Infinite loop in ZipOutputStream.close() - 8193682 : Infinite loop in ZipOutputStream.close() - 8193682 : Infinite loop in ZipOutputStream.close() - 8193682: Infinite loop in ZipOutputStream.close() - Changes: - all: https://git.openjdk.java.net/jdk/pull/5522/files - new: https://git.openjdk.java.net/jdk/pull/5522/files/c315ab21..c66535fc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5522=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5522=08-09 Stats: 937207 lines in 2953 files changed: 489188 ins; 435324 del; 12695 mod Patch: https://git.openjdk.java.net/jdk/pull/5522.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5522/head:pull/5522 PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v9]
> 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() - Changes: - all: https://git.openjdk.java.net/jdk/pull/5522/files - new: https://git.openjdk.java.net/jdk/pull/5522/files/b3d7fb74..c315ab21 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5522=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5522=07-08 Stats: 30 lines in 2 files changed: 16 ins; 0 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/5522.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5522/head:pull/5522 PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v8]
> 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() - Changes: - all: https://git.openjdk.java.net/jdk/pull/5522/files - new: https://git.openjdk.java.net/jdk/pull/5522/files/bd0ccd7c..b3d7fb74 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5522=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5522=06-07 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5522.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5522/head:pull/5522 PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v7]
On Tue, 2 Nov 2021 09:54:38 GMT, Ravi Reddy 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() Hello All . Thanks for reviewing these changes. While testing ZipOutputStream:closeEntry(), I have found the same infinite loop issue is reproducible. Since closeEntry() does internally close the deflater , even though the documentation does not specify it, I think it should be fine to give the similar fix in closeEntry() of ZipOutputStream as well. So there are total of three places where we should close deflater before throwing an exception. 1.GZipOutputStream:finish() 2.DeflaterOutputStream:close() 3.ZipOutputStream:closeEntry() I have created a CSR explaining the changes: https://bugs.openjdk.java.net/browse/JDK-8276305 - PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v7]
> 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() - Changes: - all: https://git.openjdk.java.net/jdk/pull/5522/files - new: https://git.openjdk.java.net/jdk/pull/5522/files/3154b516..bd0ccd7c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5522=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5522=05-06 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5522.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5522/head:pull/5522 PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v6]
> 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() - Changes: - all: https://git.openjdk.java.net/jdk/pull/5522/files - new: https://git.openjdk.java.net/jdk/pull/5522/files/01d84462..3154b516 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5522=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5522=04-05 Stats: 116 lines in 2 files changed: 59 ins; 20 del; 37 mod Patch: https://git.openjdk.java.net/jdk/pull/5522.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5522/head:pull/5522 PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v5]
On Thu, 28 Oct 2021 11:56:45 GMT, Ravi Reddy 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 we are closer. The formatting is better. Thank you for changing the name for the constant. Please add comments describing the intent of the test, DataProvider, and BeforeTest method to make it clear to future maintainers when they look back on the tests. We should still create a CSR to highlight the change. - PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v5]
> 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() - Changes: - all: https://git.openjdk.java.net/jdk/pull/5522/files - new: https://git.openjdk.java.net/jdk/pull/5522/files/5f1922bf..01d84462 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5522=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5522=03-04 Stats: 118 lines in 3 files changed: 11 ins; 58 del; 49 mod Patch: https://git.openjdk.java.net/jdk/pull/5522.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5522/head:pull/5522 PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v4]
On Tue, 26 Oct 2021 06:30:39 GMT, Ravi Reddy 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
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v4]
On Tue, 26 Oct 2021 06:30:39 GMT, Ravi Reddy 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() src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 247: > 245:if (usesDefaultDeflater) > 246: def.end(); > 247:throw e; The formatting is a bit wonky but I think this version is the best so far. It does mean that the stream state is undefined when close throws but this has always been the case. - PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v4]
On Tue, 26 Oct 2021 06:30:39 GMT, Ravi Reddy 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() Hi , I have made the changes in fix , I think changing existing behavior in deflate() will need a spec change. With this fix , I'm closing deflater on exception scenarios in finish method of GZipOutputStream and close method of DeflaterOutputStream. Even though the document for finsih/close methods does not clearly specifies if the deflater will be closed or not , the current behaviour of these methods does close the deflater whenever finish/close are called. \ Thanks, Ravi - PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v4]
> 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() - Changes: - all: https://git.openjdk.java.net/jdk/pull/5522/files - new: https://git.openjdk.java.net/jdk/pull/5522/files/d18eb3c1..5f1922bf Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5522=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5522=02-03 Stats: 26 lines in 2 files changed: 12 ins; 7 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/5522.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5522/head:pull/5522 PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
On Tue, 12 Oct 2021 14:46:33 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 256:
>>
>>> 254: } catch (Exception e) {
>>> 255: def.end();
>>> 256: out.close();
>>
>> out.close not needed with try with resources.
>
> This changes deflate to close the compressor and the output stream when there
> is an I/O exception. I expect this will need a spec change or a
> re-examination of the issue to see if there are alternatives.
the out.close() call could be removed I guess. Leave it for user code to handle
etc. Safer for spec also.
Main goal is to break the looping of the deflate call. The usesDefaultDeflater
boolean might be useful in helping determine if def.end() should be called or
not. If that boolean is false, then maybe we could just alter the input buffer
by setting it to a size 0 buffer (ZipUtils.defaultBuf) -- worth a look.
-
PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
On Tue, 12 Oct 2021 13:28:21 GMT, Ravi Reddy 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() Setting to "Request changes" until the spec impact is understood. - Changes requested by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
On Tue, 12 Oct 2021 14:35:17 GMT, Sean Coffey wrote:
>> src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 252:
>>
>>> 250: int len = def.deflate(buf, 0, buf.length);
>>> 251: if (len > 0) {
>>> 252: try {
>>
>> Shouldn't this use try with resources:
>> try (out) { ...
>
> the output stream is only closed if an exception is raised though ?
Yes , we are closing the stream only when exception occurs during write
operation
-
PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
On Tue, 12 Oct 2021 15:00:11 GMT, Ravi Reddy wrote: >> the output stream is only closed if an exception is raised though ? > > Yes , we are closing the stream only when exception occurs during write > operation Yes, we are closing the stream only when an exception occurs during a write operation - PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
On Tue, 12 Oct 2021 14:11:15 GMT, jmehrens wrote:
>> Ravi Reddy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8193682 : Infinite loop in ZipOutputStream.close()
>
> src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 256:
>
>> 254: } catch (Exception e) {
>> 255: def.end();
>> 256: out.close();
>
> out.close not needed with try with resources.
This changes deflate to close the compressor and the output stream when there
is an I/O exception. I expect this will need a spec change or a re-examination
of the issue to see if there are alternatives.
-
PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
On Tue, 12 Oct 2021 14:10:53 GMT, jmehrens wrote:
>> Ravi Reddy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8193682 : Infinite loop in ZipOutputStream.close()
>
> src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 252:
>
>> 250: int len = def.deflate(buf, 0, buf.length);
>> 251: if (len > 0) {
>> 252: try {
>
> Shouldn't this use try with resources:
> try (out) { ...
the output stream is only closed if an exception is raised though ?
-
PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
On Tue, 12 Oct 2021 13:28:21 GMT, Ravi Reddy 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() test/jdk/java/util/zip/GZIP/GZipLoopTest.java line 55: > 53: private static Random rand = new Random(); > 54: > 55: @Test I think we can condense the test code to aid maintenance - please consider using DataProvider - PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
On Tue, 12 Oct 2021 13:28:21 GMT, Ravi Reddy 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()
src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 252:
> 250: int len = def.deflate(buf, 0, buf.length);
> 251: if (len > 0) {
> 252: try {
Shouldn't this use try with resources:
try (out) { ...
src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 254:
> 252: try {
> 253: out.write(buf, 0, len);
> 254: } catch (Exception e) {
Shouldn't this be a finally block?
src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 256:
> 254: } catch (Exception e) {
> 255: def.end();
> 256: out.close();
out.close not needed with try with resources.
-
PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
> 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() - Changes: - all: https://git.openjdk.java.net/jdk/pull/5522/files - new: https://git.openjdk.java.net/jdk/pull/5522/files/f6a678ed..d18eb3c1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5522=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5522=01-02 Stats: 46 lines in 1 file changed: 44 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5522.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5522/head:pull/5522 PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v2]
On Mon, 11 Oct 2021 13:42:35 GMT, Ravi Reddy 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 have updated the review with the new fix. Instead of throwing an exception in close() method , Now when we get an exception during write inside deflate() , we will close the stream and throw the exception. Updated the test case in TestNG format. - PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v2]
> 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() - Changes: - all: https://git.openjdk.java.net/jdk/pull/5522/files - new: https://git.openjdk.java.net/jdk/pull/5522/files/5072b6c1..f6a678ed Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5522=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5522=00-01 Stats: 96 lines in 2 files changed: 48 ins; 28 del; 20 mod Patch: https://git.openjdk.java.net/jdk/pull/5522.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5522/head:pull/5522 PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close()
On Fri, 17 Sep 2021 12:45:29 GMT, Alan Bateman 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 > > src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 249: > >> 247: out.close(); >> 248: closed = true; >> 249: throw ioe; > > Have you tried using try-finally instead? Hi Alan , Sorry for the delayed response , I was out of office for 3 weeks. I haven't tried with try-finally , I'm reworking on the fix as , if we directly use zip.finish() , we are facing the same issue. I will update here once the fix is ready.Thanks. - PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close()
On Wed, 15 Sep 2021 07:40:35 GMT, Ravi Reddy 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 src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 249: > 247: out.close(); > 248: closed = true; > 249: throw ioe; Have you tried using try-finally instead? - PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close()
On Wed, 15 Sep 2021 07:40:35 GMT, Ravi Reddy 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 Reviewers: @LanceAndersen @coffeys - PR: https://git.openjdk.java.net/jdk/pull/5522
