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

[Jaikiran Pai]

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.

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:

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