Request for Review: 7094995: test/java/util/zip/ZipFile/ causes continuous GC in agentvm mode

Joseph Darcy joe.darcy at
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 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, the most robust pattern is to 
have one resource variable declared for each resource.


More information about the core-libs-dev mailing list