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

Thu, 24 Feb 2011 00:39:51 -0800 

 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
























					Reply via email to