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

[Xueming Shen]

  Kumar,

Would you please help review the change in your pack code?

Thanks,
-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