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
