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

[Neil Richards]

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

-----

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

[1] http://en.wikipedia.org/wiki/Idempotence
(I confess, I had to look the term up)
[2] http://cr.openjdk.java.net/~ngmr/7094995/webrev.02/

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