jackrabbit-oak-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Dürig <mdue...@apache.org>
Subject Re: Inconsistent locking in TreeImpl.Children?
Date Thu, 30 Aug 2012 13:47:14 GMT


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
>

Mime
View raw message