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

[Xueming Shen]

Neil,

It appears it might not be necessary to do the finalize() in 
ZipFileInflaterInputStream. The
ZipFileInflaterInputStream itself does not directly hold any native 
resource by itself that
needs to be released at the end of its life circle, if not closed 
explicitly. The native resource/
memory that need to be taken care of are held by its fields "inf" and 
"zfin", which should
be finalized by the corresponding finalize() of their own classes 
(again, if not closed
explicitly), when their outer ZFIIS object is unreachable. The Inflater 
class has its own finalize()
implemented already to invoke its cleanup method end(), so the only 
thing need to be addressed
is to add the finalize() into ZipFileInputStream class to call its 
close(), strictly speaking this issue
is not the regression caused by #6735255, we have this "leak" before 
#6735255.

Also, would you like to consider to use WeakHeapMap<InputStream, Void> 
instead of
handling all the weak reference impl by yourself, the bonus would be 
that the stalled entries
might be cleaned up more frequently.

Sherman


On 04/07/2011 06:30 AM, Neil Richards wrote:
> On Mon, 2011-04-04 at 09:04 +1000, David Holmes wrote:
>> 1. If a call to close() occurs around the same time as finalization
>> occurs then the finalizer thread will set inFinalizer to true, at which
>> point the thread executing close() can see it is true and so may skip
>> the releaseInflater(inf) call.
>>
>> 2. As isClosed is not volatile, and available() is not synchronized, a
>> thread calling available() on a closed stream may not see that it has
>> been closed and so will likely encounter an IOException rather than
>> getting a zero return.
>>
>>
>> Even if #1 is not a practical problem I'd be inclined to make the
>> finalizer synchronized as well. By doing that you're effectively
>> ensuring that premature finalization of the stream won't happen.
> I tend to agree, especially as it also makes the intention of the code
> clearer.
>
>> For #2 it is a tricky call. If you don't actually expect the stream
>> object to be used by multiple threads then using a synchronized block to
>> read isClosed will be cheap in VMs that go to great effort to reduce the
>> cost of unnecessary synchronization (eg biased-locking in hotspot).
>> Otherwise making isClosed volatile is likely the better option.
> The check at the start of available() guards the logic beyond (which
> uses a value from the inflater object, which would not be valid if the
> stream has been closed()).
>
> Because of this, I think it would be clearer to synchronize the
> available() method too.
>
> As you say, it is extremely likely that, in practice, this
> synchronization will never be contended, and so won't impact performance
> significantly (in modern VMs).
>
> Please find below an updated changeset with these modifications,
>
> Thanks for your advice in this,
> Neil
>