Request for Review: 7094995: test/java/util/zip/ZipFile/ClearStaleZipFileInputStreams.java causes continuous GC in agentvm mode (original) (raw)

Joseph Darcy joe.darcy at oracle.com
Fri Dec 2 00:56:48 UTC 2011

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

More information about the core-libs-dev mailing list