Cannot protect copying of synchronizedObservableMap

[Kevin Rushforth]

Hi Robert,

1. Is the behavior of SynchronizedObservableMap a bug, where it uses a 
new mutex and doesn't synchronize on itself a bug?

I'd say yes, this is a bug. Clearly the class is meant to mimic the 
corresponding Collections class with observability added. The fact that 
it then doesn't synchronize in the same way, and that this difference 
makes the class not thread-safe seems to defeat the entire purpose of 
having a synchronized flavor of ObservableMap. So go ahead and file a bug.

2. Do we need SynchronizedObservableMap at all?

If we want a map that is both synchronized and observable, then yes. In 
any case, I don't see any value in trying to eliminate it from the API 
(e.g., through deprecation --> deprecation for removal --> removal).

-- Kevin


On 10/17/2019 5:48 AM, Robert Lichtenberger wrote:
> I've just stumbled upon a devious detail in
> javafx.collections.FXCollections.SynchronizedObservableMap<K, V>. Although
> it almost looks like a twin of Collections.synchronizedMap it does not
> allow to protect copying or iterating over it the way
> Collections.synchronizedMap does.
>
> Example program:
> import java.util.Collections;
> import java.util.HashMap;
> import java.util.Map;
> import java.util.Random;
> import javafx.collections.FXCollections;
>
> public class SyncMap {
>      public static void main(String[] args) {
>          Random rnd = new Random();
>          Map<Integer, Integer> m = Collections.synchronizedMap(new
> HashMap<Integer, Integer>());
> //        Map<Integer, Integer> m =
> FXCollections.synchronizedObservableMap(FXCollections.observableHashMap());
>          Thread t = new Thread(() -> {
>              while (true) {
>                  m.put(rnd.nextInt(1000), rnd.nextInt());
>              }
>          });
>          t.setDaemon(true);
>          t.start();
>          Map<Integer, Integer> c = null;
>          for (int i = 0; i < 100000; i ++) {
>              synchronized(m) {
>                  c = new HashMap<>(m);
>              }
>          }
>          System.out.println(c);
>      }
> }
>
> Using Collections.synchronizedMap this works, because we synchronize on m.
> If using FXCollections.synchronizedObservableMap this throws
> ConcurrentModificationException. The reason is that
>          SynchronizedObservableMap(ObservableMap<K, V> map) {
>              this(map, new Object());
>          }
> SynchronizedObservableMap uses a new Object as mutex instead of itself as
> seen in
>          SynchronizedMap(Map<K,V> m) {
>              this.m = Objects.requireNonNull(m);
>              mutex = this;
>          }
>
> Questions:
> * Is this considered a bug / a possible enhancement? Should there at least
> be a warning against this possible pitfall?
> * Do we need synchronizedObservable-Stuff in FXCollections at all? This
> seems one of the cases where JavaFX contains classes/functionality that are
> part of the JDK itself.
>
> I can file an issue, if there is consensus about the issue.