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

[David Holmes]

Hi Stuart,

Just taking a look as a curious observer ...

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

Stuart Marks said the following on 02/24/11 16:26:
> 
> * 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?

> * 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().


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         }


test/java/util/zip/GZIP/GZIPInputStreamRead.java

Unroll here too:

  78         try (GZIPInputStream gzis = new GZIPInputStream(
  79                                         new ByteArrayInputStream(dst),
  80                                         gzisBufSize))
  81         {


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