review request for 7021582, use try-with-resources in jar/zip implementation and tests

[Stuart Marks]

On 2/23/11 11:11 PM, David Holmes wrote:
> Hi Stuart,
>
> Just taking a look as a curious observer ...

Good comments, thanks.

> I'm somewhat dismayed by the lack of exception handling in the original code.

Yes. As Josh observed in his initial "Automatic Resource Management" proposal 
[*] (precursor of try-with-resources), "2/3 of the uses of the close method in 
the JDK itself are wrong". I guess it's one thing to see this statement in some 
email, it's another to see actual examples littered around the code. I don't 
know if his statement is literally true, but I certainly believe that a large 
fraction of sites where code does something like opening a file do one of: a) 
failing to close the file at all; b) mishandling exceptions that occur during 
processing, for example by failing to close; or c) failing to handle exceptions 
from the close call.

This webrev represents only a subset of what findbugs and Jackpot found. I'm 
sure there are many other examples still remaining, unfortunately. But, it's a 
start!

[*] http://mail.openjdk.java.net/pipermail/coin-dev/2009-February/000011.html

>> * src/share/classes/com/sun/java/util/jar/pack/Driver.java
>>
>> I narrowed the scope of the open resource. No sense keeping it open any
>> longer than necessary. This occurs in several other places as well.
>
> You also changed from using a BufferedStream to just the FileInputStream - was
> that intentional?

Oh yes, I forgot to mention this. Properties.load() now does buffering 
internally, so it's unnecessary for callers to wrap their streams in a 
BufferedInputStream. I've seen this occur fairly frequently.

>> * src/share/classes/com/sun/java/util/jar/pack/PropMap.java
>>
>> Narrowed the scope of catch IOException; should be OK since the code that was
>> migrated out cannot throw IOException.
>
> So in this one:
>
>   128         boolean propsLoaded = false;
>   129         try (InputStream propStr =
> PackerImpl.class.getResourceAsStream(propFile)) {
>   130             props.load(propStr);
>   131             propsLoaded = true;
>   132         } catch (IOException ee) {
>   133             // ignore exception if it came from the close()
>   134             if (!propsLoaded) {
>   135                 throw new RuntimeException(ee);
>   136             }
>   137         }
>
> The RuntimeException has a cause of ee, which in turn may have a suppressed
> IOException from the close().

The intent of the original code was to convert any IOException thrown by 
opening or processing the stream into a RuntimeException, with the IOException 
as the cause. The intent was also simply to ignore an IOException from the 
close() if it occurred after successful processing. I've tried to preserve this 
intent. What you said above would occur if IOExceptions occur both in 
opening/processing and in the close(). This is somewhat odd but probably OK if 
we wanted to keep things this way.

But Alan pointed out in his comments that it's probably not worth 
special-casing the treatment of close() here, so I'll pull out the propsLoaded 
logic. He also suggested handling a null return from getResourceAsStream(). So 
I'll end up reworking this code anyway.

> test/java/util/zip/Available.java
>
> Don't you need to unroll the constructors here too:
>
>    47         try (ZipInputStream z = new ZipInputStream(new
> FileInputStream(f))) {
>    48             z.getNextEntry();
>    49             tryAvail(z);
>    50         }

Missed this one, thanks. This case was an automatically generated change from 
Jackpot but in most cases I've had to go back in and tinker with stuff manually.

> test/java/util/zip/GZIP/GZIPInputStreamRead.java
>
> Unroll here too:
>
>   78         try (GZIPInputStream gzis = new GZIPInputStream(
>   79                                         new ByteArrayInputStream(dst),
>   80                                         gzisBufSize))
>   81         {

Same thing here, maybe, or perhaps I was lazy :-) and decided that unrolling 
didn't need to be done because there's no point in ensuring that a 
ByteArrayInputStream is closed (its close method is defined to be a no-op).

I'm reconsidering this, though. It may be confusing to "unroll" the resources 
in some cases but not in others. Also, sometimes it's hard to tell whether it's 
safe to leave the resources rolled up in a cascade of constructors. Does 
try-with-resources actually provide any value at all if the underlying stream 
is a byte array instead of an external resource like a file or a socket? Or, do 
you really need to make sure that, say, a BufferedReader is closed as long as 
its underlying FileReader is closed?

I'm on the fence on this one. It may be that it's easier just to unroll every 
time instead of going through the is-it-safe analysis all the time.

s'marks

>
>
> Cheers,
> David
> ---------
>
>>
>> * src/share/classes/com/sun/java/util/jar/pack/UnpackerImpl.java
>>
>> This closes its input after successful processing. I changed this so that it
>> also closes its input if an exception is thrown.
>>
>> * test/java/util/zip/LargeZip.java
>>
>> I've "unrolled" a cascade of constructors into separate resource variables.
>> This also occurs in several other places. Basically code that used to look
>> like this:
>>
>>     ZipOutputStream zos = new ZipOutputStream(
>>         new BufferedOutputStream(
>>             new FileOutputStream(largeFile)));
>>     // process zos
>>     zos.close();
>>
>> is converted to this:
>>
>>     try (FileOutputStream fos = new FileOutputStream(largeFile);
>>          BufferedOutputStream bos = new BufferedOutputStream(fos);
>>          ZipOutputStream zos = new ZipOutputStream(bos))
>>     {
>>         // process zos
>>     }
>>
>> I think this more robust, since it closes the FileOutputStream if an
>> exception occurs during the construction of one of the stacked streams, which
>> the original code did not handle. Since the wrapper streams will close their
>> underlying streams, this will result in redundant close() calls. However,
>> close() is supposed to be idempotent so this should be OK.
>>
>> * test/java/util/zip/ZipFile/DeleteTempJar.java
>>
>> I'm not sure if this properly handles an IOException caused by
>> HttpExchange.close(). Funny, the method isn't declared to throw IOE, but this
>> test did compile and pass.
>>
>> * test/java/util/zip/ZipFile/LargeZipFile.java
>>
>> I changed this to fail the test if close() were to throw IOE. I think this is
>> proper for test code.
>>
>> * test/java/util/zip/ZipFile/ReadZip.java
>>
>> I took the liberty of converting the file copying code to use the new
>> java.nio.file.Files utilities. Well, I'm really following Alan's lead here
>> since he's prompted me to do so in other places a couple times already. :-)
>>
>> Thanks for reviewing!
>>
>> s'marks