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
