Neil,
You now have concurrency issues again. isClosed would need to be
volatile if the methods that set/check it aren't synchronized. Multiple
threads could call close() at the same time, and threads can call
available etc after close has finished its main work.
David
Neil Richards said the following on 04/08/11 22:36:
On Thu, 2011-04-07 at 16:02 -0700, Xueming Shen wrote:
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.
Hi Sherman,
Thanks for your continuing analysis of this change.
I concur with your assessment above, and agree that making the suggested
modifications to the changeset results in the code being simpler and
clearer.
Please find below an updated changeset incorporating these suggestions
(and rebased off jdk7-b136),
Let me know if you need anything else to progress this fix forward,
Thanks,
Neil
