Suspected regression: fix for 6735255 causes delay in GC of ZipFile InputStreams, increase in heap demand
xueming.shen at oracle.com
Fri Apr 1 19:07:33 UTC 2011
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?
>> 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
> Please find below an updated changeset below, containing the suggested
> change to synchronization.
> Thanks once again for your suggestions,
More information about the core-libs-dev