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

[Xueming Shen]

  Stuart,

Here are my comments on non-pack changes. I'm sure Kumar will look at 
those pack files later.

GetMethodsReturnClones.java: ln#43 diamond conersion?

Available.java: it's not your change, but I believe we should do the 
try-with_resources on the ZipFile zfile as well.

InfoZip.java.: ln#113/114 you probably don't need to separate out the 
"fis" in try-with-resources, the FileInputStream
created should be closed by the ZipInputStream that it is embedded in.

LargeZip.java: ln#101-1-4/148-151, same as above, only need to close 
ZipOut/InputStream

TestEmptyZip.java:
TestEmptyZip.java:
Comment.java: ln#60-62
CorruptedZipFiles.java ln#50
DeleteTempJar.java:
LargeZipFile.java.
ManyEntries.java
ReadZip.java.
ShortRead.java
       only close the ZipIn/OutputStream

-Sherman

On 2011-2-23 22:26, Stuart Marks wrote:
> Hi Sherman, all,
>
> Here's a webrev to convert code in the jar/zip implementation files 
> and tests to use the new Java 7 try-with-resources construct.
>
> http://cr.openjdk.java.net/~smarks/reviews/7021582/webrev.0/
>
> There are rather a lot of files, however, most of the changes are 
> pretty straightforward. A typical conversion changes code like this:
>
>     FileInputStream fis = new FileInputStream(filename);
>     // use fis
>     fis.close();
>
> to this:
>
>     try (FileInputStream fis = new FileInputStream(filename)) {
>         // use fis
>     }
>
> The majority of the conversions are like the above. However, there are 
> a several places where I had to rearrange things either in order to 
> get them to work at all, to improve robustness, or for general cleanup.
>
> Some of these are marked with "TODO". I will of course remove these 
> comments before committing the changes. Where you see these comments, 
> you might want to give the code a bit more scrutiny.
>
> Specific examples of things to look at follow:
>
> * test/java/util/jar/JarEntry/GetMethodsReturnClones.java
>
> I had to change the ordering of local variable declarations so that 
> the variable was visible where it's used. TWR introduces a nested 
> block, so obviously a local declared within will have to be moved 
> outside in order to be used outside. This occurs in several other 
> places as well. In some cases initialization order was changed. This 
> shouldn't matter, though, since it's things like opening a file vs. 
> allocating an array.
>
> * 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.
>
> * src/share/classes/com/sun/java/util/jar/pack/PackageReader.java
> * src/share/classes/com/sun/java/util/jar/pack/PackageWriter.java
>
> These changes rely on recent changes to TWR's handling of null 
> resources. Currently, TWR will avoid calling close if the resource is 
> null. Joe checked in this change just last week. Before that, a null 
> resource would generate an unavoidable NPE when it attempted to call 
> close(). Handling of non-null resources is unchanged.
>
> I don't think the change to null handling is in a promoted build yet. 
> Is it OK to check in code that depends on it? All tests pass, but that 
> just means that the path where the resource is null isn't tested.
>
> * 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.
>
> * 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