On 11/24/2011 3:15 AM, Neil Richards wrote:
On Thu, 2011-11-24 at 11:22 +1000, David Holmes wrote:
Hi Joe,
On 24/11/2011 2:33 AM, Joe Darcy wrote:
On 11/22/2011 9:57 PM, David Holmes wrote:
On 22/11/2011 9:51 PM, Neil Richards wrote:
I've also converted the testcase's use of ZipFile, ZipOutputStream&
FileOutputStream to use ARM (for greater clarity).
I think proper use of ARM requires that this:
66 try (ZipOutputStream zos =
67 new ZipOutputStream(new FileOutputStream(tempZipFile))) {
be written as:
try (FileOutputStream fos = new FileOutputStream(tempZipFile);
ZipOutputStream zos = new ZipOutputStream(fos)) {
The more conservative approach is to define one resource variable per
logical resource even if the one resource is "wrapped" by the second
one, as in the second example. This does close the small window of
vulnerability between when the first constructor call ends and the
second one completes. However, if that pattern is used, it is important
for the close method of the inner resource to be idempotent, as required
by the java.io.Closeable type but *not* required by
java.lang.AutoCloseable.
Sorry but I couldn't quite tell what you were recommending there. Is the
original code or my change the preferred approach? As I understood it
the original code would not close the FileOutputStream if the
ZipOutputStream constructor threw an exception.
Thanks,
David
In this instance, I think separating the allocations out into separate
statements in the ARM's try is fine, because both FileOutputStream and
ZipOutputStream are Closeable, and therefore have idempotent [1] close()
methods. (They're obviously also both AutoCloseable, to be used by ARM
in the first place).
-----
Consider, however, if FileOutputStream were not Closeable, and therefore
didn't guarantee the idempotency of its close().
(It might then do something like throw an Exception if close() is called
for a second time.)
Then the decision to have it auto-closed by ARM would hinge upon whether
the call to ZipOutputStream's close() causes a close() call to be made
to the (File)OutputStream it holds.
If it does, one would not want to use ARM to (also) call the
(non-Closeable) FileOutputStream's close(), as it would run the risk of
seeing non-idempotent behaviour (eg. an Exception thrown).
-----
However, coming back to reality, both objects _are_ Closeable, and so
_do_ have idempotent close() methods.
Therefore it's both safe to have them both handled by ARM, and (I'd
argue) clearer to do so, as it's then clear both objects _do_ get
closed, without having to consider the finer details of
ZipOutputStream.close().
I believe Joe was defining the considerations needed when making such a
modification in the general case.
(Joe, please correct me if I misinterpreted this).
That is correct; I was noting some subtle considerations for the more
general case.
When both resources are java.io.Closeable, the most robust pattern is to
have one resource variable declared for each resource.
-Joe
