Suspected regression: fix for 6735255 causes delay in GC of ZipFile InputStreams, increase in heap demand
David.Holmes at oracle.com
Thu Apr 7 22:13:23 UTC 2011
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
>> 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.
More information about the core-libs-dev