8232524: SynchronizedObservableMap cannot be be protected for copying…

…/iterating Reviewed-by: arapte, kcr
openjdk · Dec 5, 2019 · 46338d0 · 46338d0
1 parent a68347c
commit 46338d0
Show file tree

Hide file tree

Showing 2 changed files with 104 additions and 25 deletions.
diff --git a/modules/javafx.base/src/main/java/javafx/collections/FXCollections.java b/modules/javafx.base/src/main/java/javafx/collections/FXCollections.java
@@ -1008,6 +1008,11 @@ private static class SynchronizedList<T> implements List<T> {
             this.mutex = mutex;
         }
 
+        SynchronizedList(List<T> list) {
+            this.backingList = list;
+            this.mutex = this;
+        }
+
         @Override
         public int size() {
             synchronized(mutex) {
@@ -1197,19 +1202,15 @@ private static class SynchronizedObservableList<T> extends SynchronizedList<T> i
         private final ObservableList<T> backingList;
         private final ListChangeListener<T> listener;
 
-        SynchronizedObservableList(ObservableList<T> seq, Object mutex) {
-            super(seq, mutex);
+        SynchronizedObservableList(ObservableList<T> seq) {
+            super(seq);
             this.backingList = seq;
             listener = c -> {
                 ListListenerHelper.fireValueChangedEvent(helper, new SourceAdapterChange<T>(SynchronizedObservableList.this, c));
             };
             backingList.addListener(new WeakListChangeListener<T>(listener));
         }
 
-        SynchronizedObservableList(ObservableList<T> seq) {
-            this(seq, new Object());
-        }
-
         @Override
         public boolean addAll(T... elements) {
             synchronized(mutex) {
@@ -1774,7 +1775,8 @@ private static class SynchronizedSet<E> implements Set<E> {
         }
 
         SynchronizedSet(Set<E> set) {
-            this(set, new Object());
+            this.backingSet = set;
+            this.mutex = this;
         }
 
         @Override
@@ -1890,19 +1892,15 @@ private static class SynchronizedObservableSet<E> extends SynchronizedSet<E> imp
         private SetListenerHelper listenerHelper;
         private final SetChangeListener<E> listener;
 
-        SynchronizedObservableSet(ObservableSet<E> set, Object mutex) {
-            super(set, mutex);
+        SynchronizedObservableSet(ObservableSet<E> set) {
+            super(set);
             backingSet = set;
             listener = c -> {
                 SetListenerHelper.fireValueChangedEvent(listenerHelper, new SetAdapterChange<E>(SynchronizedObservableSet.this, c));
             };
             backingSet.addListener(new WeakSetChangeListener<E>(listener));
         }
 
-        SynchronizedObservableSet(ObservableSet<E> set) {
-            this(set, new Object());
-        }
-
         @Override
         public void addListener(InvalidationListener listener) {
             synchronized (mutex) {
@@ -2552,13 +2550,9 @@ private static class SynchronizedMap<K, V> implements Map<K, V> {
         final Object mutex;
         private final Map<K, V> backingMap;
 
-        SynchronizedMap(Map<K, V> map, Object mutex) {
-            backingMap = map;
-            this.mutex = mutex;
-        }
-
         SynchronizedMap(Map<K, V> map) {
-            this(map, new Object());
+            backingMap = map;
+            this.mutex = this;
         }
 
         @Override
@@ -2784,19 +2778,15 @@ private static class SynchronizedObservableMap<K, V> extends SynchronizedMap<K,
         private MapListenerHelper listenerHelper;
         private final MapChangeListener<K, V> listener;
 
-        SynchronizedObservableMap(ObservableMap<K, V> map, Object mutex) {
-            super(map, mutex);
+        SynchronizedObservableMap(ObservableMap<K, V> map) {
+            super(map);
             backingMap = map;
             listener = c -> {
                 MapListenerHelper.fireValueChangedEvent(listenerHelper, new MapAdapterChange<K, V>(SynchronizedObservableMap.this, c));
             };
             backingMap.addListener(new WeakMapChangeListener<K, V>(listener));
         }
 
-        SynchronizedObservableMap(ObservableMap<K, V> map) {
-            this(map, new Object());
-        }
-
         @Override
         public void addListener(InvalidationListener listener) {
             synchronized (mutex) {

diff --git a/modules/javafx.base/src/test/java/test/javafx/collections/FXCollectionsTest.java b/modules/javafx.base/src/test/java/test/javafx/collections/FXCollectionsTest.java
@@ -29,6 +29,8 @@
 import org.junit.Test;
 
 import java.util.*;
+import java.util.function.BiConsumer;
+import java.util.function.Consumer;
 import javafx.collections.FXCollections;
 import javafx.collections.ListChangeListener;
 import test.javafx.collections.MockSetObserver.Tuple;
@@ -670,6 +672,93 @@ public void checkedObservableSetTest() {
         assertEquals(5, set.size());
     }
 
+    @Test
+    public void synchronizedMapIterationProtectionTest() {
+        testIterationProtection(FXCollections.synchronizedObservableMap(FXCollections.observableHashMap()), this::putRandomValue, this::copyMap);
+    }
+
+    private void putRandomValue(Map<Integer, Integer> map, Random rnd) {
+        map.put(rnd.nextInt(1000), rnd.nextInt());
+    }
+
+    private void copyMap(Map<Integer, Integer> map) {
+        new HashMap<>(map);
+    }
+
+    @Test
+    public void synchronizedSetIterationProtectionTest() {
+        testIterationProtection(FXCollections.synchronizedObservableSet(FXCollections.observableSet()), this::addRandomValue, this::copySet);
+    }
+
+    private void addRandomValue(Set<Integer> set, Random rnd) {
+        set.add(rnd.nextInt(1000));
+    }
+
+    private void copySet(Set<Integer> set) {
+        new HashSet<>(set);
+    }
+
+    @Test
+    public void synchronizedListIterationProtectionTest() {
+        testIterationProtection(FXCollections.synchronizedObservableList(FXCollections.observableArrayList()), this::modifyList, this::iterateOverList);
+    }
+
+    private void modifyList(List<Integer> list, Random rnd) {
+        if (rnd.nextInt(1000) > list.size()) {
+            list.add(rnd.nextInt(1000));
+        } else {
+            list.remove(rnd.nextInt(list.size()));
+        }
+    }
+
+    private void iterateOverList(List<Integer> list) {
+        Iterator<Integer> it = list.iterator();
+        while (it.hasNext()) {
+            it.next();
+        }
+    }
+
+    public <V> void testIterationProtection(V collection, BiConsumer<V, Random> backgroundChanger, Consumer<V> protectedCode) {
+        CollectionChangeThread<V> thread = new CollectionChangeThread<>(collection, backgroundChanger);
+        thread.start();
+        for (int i = 0; i < 10000; i++) {
+            try {
+                synchronized (collection) {
+                    protectedCode.accept(collection);
+                }
+            } catch (ConcurrentModificationException e) {
+                thread.terminate();
+                fail("ConcurrentModificationException should not be thrown");
+            }
+        }
+        thread.terminate();
+    }
+
+    private static class CollectionChangeThread<V> extends Thread {
+        private boolean shallRun = true;
+        private V collection;
+        private BiConsumer<V, Random> backgroundChanger;
+        private Random rnd = new Random();
+
+        public CollectionChangeThread(V collection, BiConsumer<V, Random> backgroundChanger) {
+            super("FXCollectionsTest.CollectionChangeThread");
+            this.collection = collection;
+            this.backgroundChanger = backgroundChanger;
+        }
+
+        @Override
+        public void run() {
+            while (shallRun) {
+                backgroundChanger.accept(collection, rnd);
+            }
+        }
+
+        public void terminate() {
+            shallRun = false;
+        }
+    }
+
+
     private static class NonSortableObservableList extends AbstractList<String> implements ObservableList<String> {
 
         private List<String> backingList = new ArrayList<String>();