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

[David Holmes]

Hi Neil,

Neil Richards said the following on 04/07/11 23:30:
> 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,

No problem. From a synchronization perspective this all seems fine now.

Cheers,
David