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

[Lance Andersen]

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> 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/20200205/c5f102e8/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/20200205/c5f102e8/oracle_sig_logo-0001.gif>