On 30.8.12 14:09, Marcel Reutegger wrote:
On 28.8.12 16:37, Marcel Reutegger wrote:
TreeImpl.Children.iterator() returns an iterator over the values of the
internal children Map without locking. Other methods accessing the children
Map use read or write locks, depending on the operation.
I'm wondering if locking is missing there or if it is even needed.
The thinking is that the iterators returned from Tree have snapshot
semantics. That is, they represent the state of the underlying tree at
the point the iterator was returned. Later changes are not reflected nor
do mutating calls to the tree from within such an iterator result in a
ConcurrentModificationException. See the Javadoc for Tree.getChildren.
While the initial design might have deteriorated to some degree I think
the general contract still holds. Furthermore locking should not be
necessary since by contract Tree instances are not thread safe.
The JavaDoc currently says that it is not thread-save for writing, but
it is for reading. I guess that's the reason why there is locking on the
Children. AFAIU children are loaded lazily and if we allow concurrent
reads we will have a situation where children are potentially added
concurrently to that map.
Right.
It also seems the locking is a left over from the previous children
implementation class that was used before Oak switched to Google
Guava. Previously it was using ReferenceMap, which is not thread-safe,
but the current implementation *is* thread-safe. See JavaDoc of
Cache.asMap(). I think the locking can be removed from Children.
Yes good catch!
BTW. Children.iterator is currently never called at all. It is a
leftover from an earlier implementation where the caller ensured the
contract described above would hold.
Then I'd suggest we remove that method as well.
Hmm, with those changes Children becomes a thin wrapper for
three methods on a Map. I guess we should get rid of Children
at this point. I'll create an issue and attach a patch.
Works for me. However see also Jukka's reply. He came up with an
ingenious way which doesn't rely on weak references at all.
Michael
Regards
Marcel
