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

[Xueming Shen]

  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
>