Suspected regression: fix for 6735255 causes delay in GC of ZipFile InputStreams, increase in heap demand

[Xueming Shen]

On 03/30/2011 12:53 PM, Neil Richards wrote:
> Hi Sherman,
> Thanks for your review and comments on this.
>
> On Tue, 2011-03-29 at 12:05 -0700, Xueming Shen wrote:
>> Hi Neil,
>>
>> It appears to be a "regression" in scenario you described (user
>> application never close the input stream after use and the ZipFile
>> instance being retained during the lifetime of the process). The
>> proposed approach seems to solve this particular problem.
>>
>> Here are my comments regarding the patch.
>>
>> (1) ZipFileInflaterInputStream.close() now synchronizes on
>>        Zipfile.this, is it really necessary?
> Synchronization is used so that an update to 'isClosed' on one thread is
> seen by another.
>
> Adding finalization (ie. a call from finalize() to close()) effectively
> introduces a new thread into the mix, namely the finalization thread.
> Whilst you're correct in saying that, in general, InputStream gives no
> thread-safe guarantees, in this particular case I believe it is valid to
> make sure updates to this field are seen across threads, to prevent the
> logic of close() being run twice.

Isn't it true that when the finalize()->close() gets invoked, there 
should be no strong
reference anywhere else that you can use to invoke close() in other thread?
ZipFileInputStream class has to sync on ZipFile.this, the read/close() 
methods are
accessing the underlying/shared bits of the ZipFile. For 
ZipFileInflaterInputStream.close()
case, even we want to make sure the isClose is synced,  I would prefer 
to see a "local"
lock, maybe simply make close() synchronized is better?

-Sherman

> I chose to synchronize on ZipFile.this merely for consistency - ie.
> because that's what ZipFileInputStream (the other InputStream impl in
> ZipFile) does.
>
> I suppose I felt the risks of creating some kind of obscure deadlock
> scenario was minimised by copying the existing pattern.
>
> I can be easily swayed if you think there's a better object to
> synchronize on, that doesn't run an increased risk of deadlock.
>
>> (2) Is there any particular reason that we don't want to "reuse" the
>>      Inflater(), if the close comes from the finalize() in your testing
>>      evn? One thing we noticed before is that the moment the
>>      InputStream gets finalized, the Inflater embedded might have been
>>      finalized already, this is the reason why we have a "inf.ended()"
>>      check in releaseInflater().
> The ZipFileInflaterInputStream object's finalize() method is called by
> the finalization thread, after it has been added to the finalization
> queue by GC.
>
> If *it* has been added to the finalization queue (because it's eligible
> for GC), then so will the Inflater object it is referencing.
>
> As finalization gives no guarantees as to the order in which objects are
> finalized, regardless of whether finalize() has been called on the
> Inflater object, it is nevertheless the case that its method *will* be
> called at some point, so the Inflater object *will* be ended.
>
> If it has been put back into the inflater cache (because ZFIIS's
> finalize happened to be called first), then this ended inflater will lie
> in wait for another ZFIIS to use it, which would result in a
> particularly unexpected exception being thrown.
>
> In general, I believe it to be quite frowned on to "resurrect" objects
> which are going through finalization (including those queued up for
> finalization) and place them back into the live set of object, as weird
> behaviour often ensues if one does.
>
> (Especially as, iirc, finalize() is only called once per object,
> regardless of whether it is "dug back up" in this fashion).
>
>> (3) The attached test case does "f.close()" in its finally block, I
>> guess it should be f.delete()?
> When I wrote the testcase, I relied upon File.deleteOnExit() to take
> care of cleaning up the temporary file(s) created.
>
> However, I recall now that I don't think it guarantees deletion on
> abnormal termination.
>
> I agree that calling f.delete() instead of f.close() would be a good
> improvement to the testcase.
>
> Please find below an updated changeset, with this change made and the
> bug number filled in.
>
> Hope this helps to clarify things,
> Thanks again,
> Neil
>