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

[Joseph Darcy]

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