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

Neil Richards neil.richards at
Thu Nov 24 11:15:28 UTC 2011

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

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).


If I'm correct on this, then I think my suggested change [2] is still
good to go.
Please speak up if there remain issues with it.

Regards, Neil

(I confess, I had to look the term up)

Unless stated above:
IBM email: neil_richards at
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

More information about the core-libs-dev mailing list