Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8232524: SynchronizedObservableMap cannot be be protected for copying/iterating #17

Closed
wants to merge 3 commits into from
Closed
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -1008,6 +1008,11 @@ public void remove(int from, int to) {
this.mutex = mutex;
}

SynchronizedList(List<T> list) {
this.backingList = list;
this.mutex = this;
}

@Override
public int size() {
synchronized(mutex) {
@@ -1197,19 +1202,15 @@ public boolean equals(Object o) {
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 @@ public void clear() {
}

SynchronizedSet(Set<E> set) {
this(set, new Object());
this.backingSet = set;
this.mutex = this;
}

@Override
@@ -1890,19 +1892,15 @@ public int hashCode() {
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 @@ public boolean equals(Object o) {
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 @@ public void clear() {
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) {
@@ -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");
This conversation was marked as resolved by effad

This comment has been minimized.

Copy link
@arapte

arapte Dec 3, 2019

The thread should be terminated here too, please add thread.terminate(); before fail()

This comment has been minimized.

Copy link
@effad

effad Dec 4, 2019

Author

You're right. I just pushed the fix.

}
}
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>();
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.