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
