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

[dmytro sheyko]

Thank you, I got it. I have one more a bit unrelated question.

The checked, synchronized and unmodifiable wrappers in some cases store
backing collection in more than one fields.

Thus `UnmodifiableList<E>` has
1. its own field `List<? extends E> list` and
2. `Collection<? extends E> c`, which it inherits from
`UnmodifiableCollection<E>`.

Also `UnmodifiableNavigableSet<E>` has even 3 duplicated fields:
1. its own `NavigableSet<E> ns`,
2. `SortedSet<E> ss` from `UnmodifiableSortedSet<E>` and
3. `Collection<? extends E> c` from `UnmodifiableCollection<E>`.

Isn't it worth to get rid of such duplication? (at least for unmodifiable
collections). This may affect serialization, but I believe it's still
possible preserve serialization backward compatible, if it's necessary.
Or is it done intentionally?

Thank you,
Dmytro

On Fri, May 1, 2020 at 1:20 AM Stuart Marks <stuart.marks at oracle.com> wrote:

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