Skip to content

Commit

Permalink
8232524: SynchronizedObservableMap cannot be be protected for copying…
Browse files Browse the repository at this point in the history 
…/iterating

Reviewed-by: arapte, kcr
  • Loading branch information
@kevinrushforth
Robert Lichtenberger authored and kevinrushforth committed Dec 5, 2019
1 parent a68347c commit 46338d0
Show file tree
Hide file tree
Showing 2 changed files with 104 additions and 25 deletions.
40 changes: 15 additions & 25 deletions modules/javafx.base/src/main/java/javafx/collections/FXCollections.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
89 changes: 89 additions & 0 deletions modules/javafx.base/src/test/java/test/javafx/collections/FXCollectionsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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>();
Expand Down

0 comments on commit 46338d0

Please sign in to comment.