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

[David Holmes]

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
>