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
