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

[Xueming Shen]

  Hi Neil,

All tests passed.

I'm starting to push your last patch. I generated the webrev at

http://cr.openjdk.java.net/~sherman/7031076/webrev/

It should be exactly the same as your last patch.

Thanks,
Sherman

On 4/18/2011 9:49 AM, Xueming Shen wrote:
>  On 4/18/2011 5:33 AM, Neil Richards wrote:
>> On Thu, 2011-04-14 at 14:48 -0700, Xueming Shen wrote:
>>> Opinion? anything I overlooked/missed?
>> Hi Sherman,
>> Thanks once more for all your help and advice on this - I'm in favour of
>> almost all of what you suggest. :-)
>>
>> I think it's worthwhile trying to clear 'streams' entries from a
>> finalize method in ZFIIS, to help ensure they are cleared in a timely
>> manner.
>>
>> I don't think the Inflater objects need to be held softly in
>> 'inflaterCache', as they will have been reset() at the point they are
>> put into that cache (in releaseInflater()).
>>
>> (I was holding them softly due to a worry over any delay in clearing
>> their 'buf' reference, which is done when reset() is called. Now that
>> the 'streams' entries are being cleared from close() and finalize() -
>> ie. up front - I think this worry is adequately addressed.)
>>
>>
>> I'm worried about changing the object 'streams' refers to in
>> ZipFile.close(), as threads are using that object to synchronize
>> against.
>> I don't believe it is currently giving the guarantee against
>> ConcurrentModificationException being seen from the iterator that I
>> believe you intend it to be, as other threads, calling (for example)
>> ZFIIS.close() at the same time will not be guaranteed to see the update
>> to the value of 'streams'.
>>
>> Instead, I've modified this code so that a real copy of 'streams' is
>> produced, by calling 'new HashMap<>(streams)'. It is this copy that is
>> iterated through to close() the InputStreams and end() the Inflaters.
>> Once the copy is obtained, 'streams' can be safely clear()'d.
>>
>> Because it is not clear that a Collections.synchronizedMap will give
>> consistent results when fed into the constructor of HashMap, I've used
>> explicit synchronization on 'streams' instead, to ensure 'streams' isn't
>> modified during the construction of 'copy'.
>> As each of the calls to InputStream.close() will synchronize on
>> 'streams' to call its remove() method, I've held that monitor whilst
>> those calls are made.
>
> (resent, got some mail issue)
>
> Hi Neil,
>
> If you are now explicitly synchronize on "streams" everywhere, I don't 
> think we even need a copy
> at close().
>
> To add "close() in ZFIIS.finalize() appears to be safe now (?, maybe I 
> should think it again), but
> given we are now using WeakHashMap, those weak reference will get 
> expunged every time that
> map gets accessed, I doubt that really help lots (have to wait for the 
> GC kicks in anyway, if not
> closed explicitly)
>
> I'm running tests now, with the "copy" mentioned above.
>
> -Sherman
>
>
>>
>> Other minor tweaks are changing 'isClosed' fields to
>> 'closeRequested' (to better describe their use now), changing
>> 'ZipFile.closeRequested' to be volatile (so it can be checked before
>> synchronization, and so that it conforms to the pattern now established
>> elsewhere in the file), and reducing the synchronization block in
>> releaseInflater() (only the call to 'inflateCache.add()' need be
>> protected by synchronization on 'inflaterCache').
>>
>> Please find below the resultant changeset,
>> Let me know what you think,
>> Thanks,
>> Neil
>>
>