Collections.synchronizedXXX() and internal mutex (aka SyncRoot)

[Stuart Marks]

The general rule in the collections that wrappers and views don't divulge their 
backing collections. The reason is fairly obvious for things like the checked 
wrappers and unmodifiable wrappers, but it's equally important for various view 
collections like those of Lists, Sets, and Maps. If there were something like 
getSyncRoot() it could be used to break encapsulation. For example:

     Map<K, V> map = Collections.synchronizedMap(...);
     someMethod(map.values());

Currently, the caller is assured that someMethod() can only see the values of 
the map. Also, as you observe, someMethod() can't safely iterate that collection 
using an iterator or stream. If getSyncRoot() were introduced to address that issue,

     void someMethod(Collection<V> values) {
         @SuppressWarnings("unchecked")
         var map = (Map<K,V>) Collections.getSyncRoot(values);
         // I now have access to map's keys and can put new entries, mwahaha!
     }

Undoubtedly there are ways we can avoid this, but the cost is designing yet more 
complicated APIs in an area where it provides little value. I think it's pretty 
unlikely that we'll do anything like this, or variants that allow the caller to 
provide an external lock at creation time, such as proposed in JDK-4335520. 
There's a pretty fundamental clash between external locking and encapsulation, 
and the platform has shifted to pursuing other approaches for concurrency.

Legacy code that needs to iterate collection views might find some luck 
replacing iterator loops with bulk methods like forEach() or removeIf().

s'marks


On 4/30/20 1:34 AM, dmytro sheyko wrote:
> Hi Stuart,
> 
> In general I agree with you regarding synchronized collections. But nevertheless 
> there is a lot of legacy code that still uses synchronized wrappers. And 
> sometimes synchronization is done on wrong lock object, which leads to 
> relatively rare but extremely hard to reproduce and troubleshoot errors. 
> Reworking whole this legacy stuff is risky. But if we had means to get the right 
> lock object for given synchronized collections, this would help us to make the 
> fixes more localized and hence safe. That's why I would like to know the reason, 
> why this has not been done earlier, and is there hope/plan this will be done in 
> near future.
> 
> Thank you,
> Dmytro
> 
> On Thu, Apr 30, 2020 at 6:36 AM Stuart Marks <stuart.marks at oracle.com 
> <mailto:stuart.marks at oracle.com>> wrote:
> 
>     Hi Dmytro,
> 
>     Callers of an API performing explicit synchronization, along with the
>     synchronized collections wrappers, have mostly fallen into disuse since the
>     introduction of the java.util.concurrent collections.
> 
>     Multiple threads can either interact directly on a concurrent collection, or
>     the
>     developer can provide an intermediate object (not a collection) that does
>     internal locking, and that exports the right set of thread-safe APIs to
>     callers.
>     I'm thus skeptical of the utility of enhancing these wrapper classes with
>     additional APIs.
> 
>     Do you have a use case that's difficult to handle any other way?
> 
>     s'marks
> 
> 
> 
>     On 4/29/20 12:58 AM, dmytro sheyko wrote:
>      > Hello,
>      >
>      > Have you ever discussed to make field mutex in synchronized collections
>      > accessible?
>      >
>      > Javadoc for Collections#synchronizedSortedSet suggest to iterate collection
>      > this way:
>      >
>      >    SortedSet s = Collections.synchronizedSortedSet(new TreeSet());
>      >    SortedSet s2 = s.headSet(foo);
>      >        ...
>      >    synchronized (s) {  // Note: s, not s2!!!
>      >        Iterator i = s2.iterator(); // Must be in the synchronized block
>      >        while (i.hasNext())
>      >            foo(i.next());
>      >    }
>      >
>      > I.e. in order to iterate subset, we also need a reference to the whole set,
>      > which is not really convenient. How about to make it possible to write:
>      >
>      >    SortedSet s2 = s.headSet(foo);
>      >        ...
>      >    synchronized (Collections.getSyncRoot(s2)) {  // Note:
>      > Collections.getSyncRoot(s2)
>      >        Iterator i = s2.iterator(); // Must be in the synchronized block
>      >        while (i.hasNext())
>      >            foo(i.next());
>      >    }
>      >
>      > Also I think it would be convenient to let to provide custom sync root when
>      > synchronized collection is created.
>      > E.g.
>      >
>      >    Object customSyncRoot = new Object();
>      >    SortedSet s = Collections.synchronizedSortedSet(new TreeSet(),
>      > customSyncRoot);
>      >
>      > What do you think about this?
>      >
>      > Regards,
>      > Dmytro
>      >
>