RFR: JDK-8225507: (zipfs): No space left on device not thrown with ZipFileSystem

[Jaikiran Pai]

Thank you Lance for the review.

Anyone else have any reviews and thoughts especially around the
Files.delete(...) API call noted in the mail?

-Jaikiran

On 06/02/20 5:04 am, Lance Andersen wrote:
> Hi Jaikiran,
>
> I think overall the patch is correct but it would be good to get some
> extra eyes on this.
>
>> On Jan 31, 2020, at 1:32 AM, Jaikiran Pai <jai.forums2013 at gmail.com
>> <mailto:jai.forums2013 at gmail.com>> wrote:
>>
>> Initial discussion about this was done here
>> https://mail.openjdk.java.net/pipermail/nio-dev/2020-January/007014.html
>>
>> I looked into this in a bit more detail. The first place where this
>> IOException is caught and swallowed involves the following code:
>>
>> try {
>>     if (e.type == Entry.COPY) {
>>         // entry copy: the only thing changed is the "name"
>>         // and "nlen" in LOC header, so we update/rewrite the
>>         // LOC in new file and simply copy the rest (data and
>>         // ext) without enflating/deflating from the old zip
>>         // file LOC entry.
>>         if (buf == null)
>>             buf = new byte[8192];
>>         written += copyLOCEntry(e, true, os, written, buf);
>>     } else {                          // NEW, FILECH or CEN
>>         e.locoff = written;
>>         written += e.writeLOC(os);    // write loc header
>>         written += writeEntry(e, os);
>>     }
>>     elist.add(e);
>> } catch (IOException x) {
>>     x.printStackTrace();    // skip any in-accurate entry
>> }
>>
>> In this, the copyLOCEntry(...), the writeLOC(...) and the
>> writeEntry(...) methods can potentially throw the IOException. Looking
>> at each of these methods,
>>
>>     - The copyLOCEntry throws an IOException if it fails to write out
>> data to the OutputStream. It additionally throws a ZipException (which
>> is an IOException) if it fails to read a LOC.
>>
>>     - The Entry#writeLOC method takes a OutputStream and writes out LOC
>> data to that stream and can potentially throw an IOException if the
>> writes to the stream fail.
>>
>>     - The writeEntry method writes out the actual data to the
>> OutputStream and uses the "transferTo" API. Additionally this method
>> also deletes (any) temp file that might have been created for holding
>> the "Entry" content. Either of these cases can throw an IOException.
>>
>> So in all these above 3 methods, the potential IOException(s) are all
>> genuine issue that I think should propagate back. The only one that I'm
>> unsure is the case in writeEntry, where it does a Files.delete(e.file)
>> where e.file is any temporary file that was created previously to hold
>> on to the entry data. Should any IOException on this line propagate
>> back? Perhaps it should, or else it would potentially leave the temp
>> file (containing the actual data) around without the user knowing that
>> it got left behind.
>
> I think this is something we need to decide.  I could argue this both
> ways.
>>
>> The other place where the IOException gets swallowed is this block:
>>
>> try {
>>     if (buf == null)
>>         buf = new byte[8192];
>>     written += copyLOCEntry(e, false, os, written, buf);
>>     elist.add(e);
>> } catch (IOException x) {
>>     x.printStackTrace();    // skip any wrong entry
>> }
>>
>> In this case the only potential call which can trigger the IOException
>> is the copyLOCEntry, which is actually the same method that we analyzed
>> above.
>>
>> So overall, I think propagating these IOException(s) should be fine
>> since they expose any real underlying issue.
>>
>> I've created a webrev which includes this change and is placed here[1].
>> If the idea to propagate this exception back and the above analysis
>> seems right, can I get a review and a sponsor for the patch please?
>>
>> Given the nature of this issue, I couldn't think of any easy and
>> consistent test case to add for this change. However, I have ran the
>> test/jdk/jdk/nio/ jtreg tests and they have allowed passed:
>
> I don’t believe there is a way to automate a test for this.  
>
> Best
> Lance
>>
>> jtreg -jdk:build/macosx-x86_64-server-release/images/jdk
>> test/jdk/jdk/nio/
>>
>>
>> Test results: passed: 21
>>
>>
>> [1] https://cr.openjdk.java.net/~jpai/webrev/8225507/1/webrev/
>>
>> -Jaikiran
>>
>>
>>
>
> <http://oracle.com/us/design/oracle-email-sig-198324.gif>
> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>
> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance
> Andersen| Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering 
> 1 Network Drive 
> Burlington, MA 01803
> Lance.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20200207/fb03fe9a/attachment-0001.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: oracle_sig_logo.gif
Type: image/gif
Size: 658 bytes
Desc: not available
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20200207/fb03fe9a/oracle_sig_logo-0001.gif>