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

[David Holmes]

Xueming Shen said the following on 04/02/11 05:07:
> On 04/01/2011 09:42 AM, Neil Richards wrote:
>> On Wed, 2011-03-30 at 13:31 -0700, Xueming Shen wrote:
>>> 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?
>> It's true that once finalize() has been called, there can't be another
>> explicit call to close().
>> However, if close() is explicitly called first, it will be called again
>> when finalize() calls it, so one still wants the update to 'isClosed' to
>> be seen by the finalizer thread (in this case).
> 
> I'm not a GC guy, so I might be missing something here, but if close() 
> is being explicitly
> invoked by some thread, means someone has a strong reference to it, I 
> don't think the
> finalize() can kick in until that close() returns and the strong 
> reference used to make that
> explicit invocation is cleared. The InputStream is eligible for 
> finalization only after it is
> "weakly" reachable, means no more "stronger" reachable exists, right?

Actually no. One of the more obscure corner cases with finalization is 
that you can actually finalize an object that is still being used. The 
JLS actually spells this out - see section 12.6.1 and in particular the 
Discussion within that section.

David

> -sherman
> 
>>> 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?
>> After assessing it again, I tend to agree - synchronizing on the ZFIIS
>> (instead of the ZipFile) now looks to me to be safe because it does not
>> do anything which synchronizes on the ZipFile object.
>>
>> (Note that if it _did_ cause such synchronization, it would risk
>> deadlock with the code in ZipFile's close() method, which is
>> synchronized on itself when it calls close() on each remaining ZFIIS.
>> It's the difference in the order of acquiring the monitors that would
>> cause the potential for deadlock).
>>
>> As it is, both close() methods synchronize on the 'inflaters' object
>> within their respective synchronized blocks, but that does not risk
>> deadlock.
>>
>> Please find below an updated changeset below, containing the suggested
>> change to synchronization.
>>
>> Thanks once again for your suggestions,
>> Neil
>>
>