jackrabbit-oak-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Marcel Reutegger <mreut...@adobe.com>
Subject RE: Inconsistent locking in TreeImpl.Children?
Date Thu, 30 Aug 2012 13:09:59 GMT
> 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.

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.

> 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.


View raw message