Bug 532273 - [DataBinding] Race condition between Binding.doUpdate and
Observable.dispose with multiple realms
Change-Id: I4136f4077267c28e8345df082f68d1589cdfba92
Signed-off-by: Conrad Groth <info@conrad-groth.de>
diff --git a/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/Binding.java b/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/Binding.java
index b8eaaef..37268ca 100644
--- a/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/Binding.java
+++ b/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/Binding.java
@@ -181,4 +181,18 @@
return Observables.staticObservableList(context.getValidationRealm(),
Collections.singletonList(model));
}
+
+ /**
+ * @param observable the observable that is checked for disposal before the
+ * runnable gets executed.
+ * @param r the Runnable to execute in the observable's realm.
+ * @since 1.6
+ */
+ protected final void execAfterDisposalCheck(final IObservable observable, final Runnable r) {
+ observable.getRealm().exec(() -> {
+ if (!observable.isDisposed()) {
+ r.run();
+ }
+ });
+ }
}
diff --git a/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/ListBinding.java b/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/ListBinding.java
index a0fac10..2326763 100644
--- a/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/ListBinding.java
+++ b/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/ListBinding.java
@@ -85,7 +85,7 @@
@Override
protected void postInit() {
if (modelToTarget.getUpdatePolicy() == UpdateListStrategy.POLICY_UPDATE) {
- getModel().getRealm().exec(() -> {
+ execAfterDisposalCheck(getModel(), () -> {
((IObservableList) getModel()).addListChangeListener(modelChangeListener);
updateModelToTarget();
});
@@ -94,7 +94,7 @@
}
if (targetToModel.getUpdatePolicy() == UpdateListStrategy.POLICY_UPDATE) {
- getTarget().getRealm().exec(() -> {
+ execAfterDisposalCheck(getTarget(), () -> {
((IObservableList) getTarget()).addListChangeListener(targetChangeListener);
if (modelToTarget.getUpdatePolicy() == UpdateListStrategy.POLICY_NEVER) {
// we have to sync from target to model, if the other
@@ -113,7 +113,7 @@
@Override
public void updateModelToTarget() {
final IObservableList modelList = (IObservableList) getModel();
- modelList.getRealm().exec(() -> {
+ execAfterDisposalCheck(modelList, () -> {
ListDiff diff = Diffs.computeListDiff(Collections.EMPTY_LIST, modelList);
doUpdate(modelList, (IObservableList) getTarget(), diff, modelToTarget, true, true);
});
@@ -122,7 +122,7 @@
@Override
public void updateTargetToModel() {
final IObservableList targetList = (IObservableList) getTarget();
- targetList.getRealm().exec(() -> {
+ execAfterDisposalCheck(targetList, () -> {
ListDiff diff = Diffs.computeListDiff(Collections.EMPTY_LIST, targetList);
doUpdate(targetList, (IObservableList) getModel(), diff, targetToModel, true, true);
});
@@ -156,7 +156,7 @@
*/
diff.getDifferences();
}
- destination.getRealm().exec(() -> {
+ execAfterDisposalCheck(destination, () -> {
if (destination == getTarget()) {
updatingTarget = true;
} else {
diff --git a/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/SetBinding.java b/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/SetBinding.java
index dc6279e..30d65f0 100644
--- a/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/SetBinding.java
+++ b/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/SetBinding.java
@@ -87,7 +87,7 @@
@Override
protected void postInit() {
if (modelToTarget.getUpdatePolicy() == UpdateSetStrategy.POLICY_UPDATE) {
- getModel().getRealm().exec(() -> {
+ execAfterDisposalCheck(getModel(), () -> {
((IObservableSet) getModel()).addSetChangeListener(modelChangeListener);
updateModelToTarget();
});
@@ -96,7 +96,7 @@
}
if (targetToModel.getUpdatePolicy() == UpdateSetStrategy.POLICY_UPDATE) {
- getTarget().getRealm().exec(() -> {
+ execAfterDisposalCheck(getTarget(), () -> {
((IObservableSet) getTarget()).addSetChangeListener(targetChangeListener);
if (modelToTarget.getUpdatePolicy() == UpdateSetStrategy.POLICY_NEVER) {
// we have to sync from target to model, if the other
@@ -114,7 +114,7 @@
@Override
public void updateModelToTarget() {
final IObservableSet modelSet = (IObservableSet) getModel();
- modelSet.getRealm().exec(() -> {
+ execAfterDisposalCheck(modelSet, () -> {
SetDiff diff = Diffs.computeSetDiff(Collections.EMPTY_SET, modelSet);
doUpdate(modelSet, (IObservableSet) getTarget(), diff, modelToTarget, true, true);
});
@@ -123,7 +123,7 @@
@Override
public void updateTargetToModel() {
final IObservableSet targetSet = (IObservableSet) getTarget();
- targetSet.getRealm().exec(() -> {
+ execAfterDisposalCheck(targetSet, () -> {
SetDiff diff = Diffs.computeSetDiff(Collections.EMPTY_SET, targetSet);
doUpdate(targetSet, (IObservableSet) getModel(), diff, targetToModel, true, true);
});
@@ -160,7 +160,7 @@
diff.getAdditions();
diff.getRemovals();
}
- destination.getRealm().exec(() -> {
+ execAfterDisposalCheck(destination, () -> {
if (destination == getTarget()) {
updatingTarget = true;
} else {
diff --git a/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/ValueBinding.java b/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/ValueBinding.java
index 35ce5ad..87eebd3 100644
--- a/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/ValueBinding.java
+++ b/bundles/org.eclipse.core.databinding/src/org/eclipse/core/databinding/ValueBinding.java
@@ -159,7 +159,7 @@
if (policy == UpdateValueStrategy.POLICY_ON_REQUEST && !explicit)
return;
- source.getRealm().exec(() -> {
+ execAfterDisposalCheck(source, () -> {
boolean destinationRealmReached = false;
final MultiStatus multiStatus = BindingStatus.ok();
try {
@@ -190,7 +190,7 @@
// Set value
destinationRealmReached = true;
- destination.getRealm().exec(() -> {
+ execAfterDisposalCheck(destination, () -> {
if (destination == target) {
updatingTarget = true;
} else {
diff --git a/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/databinding/observable/ThreadRealm.java b/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/databinding/observable/ThreadRealm.java
index 351b73e..ec6cb85 100755
--- a/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/databinding/observable/ThreadRealm.java
+++ b/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/databinding/observable/ThreadRealm.java
@@ -28,6 +28,7 @@
private final LinkedList<Runnable> queue = new LinkedList<Runnable>();
private volatile boolean block;
+ private volatile boolean doProcess;
/**
* Initializes the realm.
@@ -90,12 +91,15 @@
try {
synchronized (queue) {
+ doProcess = true;
+ queue.notifyAll();
while (!queue.isEmpty()) {
if (!block)
throw new IllegalStateException(
"Cannot process queue, ThreadRealm is not blocking on its thread");
queue.wait();
}
+ doProcess = false;
}
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
@@ -127,13 +131,14 @@
while (block) {
Runnable runnable = null;
- synchronized (queue) {
- if (queue.isEmpty()) {
- queue.wait();
- } else {
- runnable = queue.getFirst();
- }
+ while (!doProcess) {
+ synchronized (queue) {
+ queue.wait();
+ }
}
+ synchronized (queue) {
+ runnable = queue.peek();
+ }
if (runnable != null) {
safeRun(runnable);
diff --git a/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/internal/databinding/DifferentRealmsBindingTest.java b/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/internal/databinding/DifferentRealmsBindingTest.java
index f2d141a..b35985c 100644
--- a/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/internal/databinding/DifferentRealmsBindingTest.java
+++ b/tests/org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/internal/databinding/DifferentRealmsBindingTest.java
@@ -20,9 +20,9 @@
import java.util.concurrent.atomic.AtomicReference;
import org.eclipse.core.databinding.DataBindingContext;
+import org.eclipse.core.databinding.observable.IObservable;
import org.eclipse.core.databinding.observable.list.ComputedList;
import org.eclipse.core.databinding.observable.list.IObservableList;
-import org.eclipse.core.databinding.observable.list.ObservableList;
import org.eclipse.core.databinding.observable.list.WritableList;
import org.eclipse.core.databinding.observable.set.ComputedSet;
import org.eclipse.core.databinding.observable.set.IObservableSet;
@@ -41,8 +41,10 @@
public class DifferentRealmsBindingTest {
- ThreadRealm targetAndModelRealm = new ThreadRealm();
ThreadRealm validationRealm = new ThreadRealm();
+ ThreadRealm targetRealm = new ThreadRealm();
+ ThreadRealm modelRealm = new ThreadRealm();
+ ThreadRealm mainThread = new ThreadRealm();
List<IStatus> errorStatusses = new ArrayList<>();
@@ -59,54 +61,65 @@
@Before
public void setUp() throws Exception {
errorStatusses.clear();
- new Thread() {
- @Override
- public void run() {
- targetAndModelRealm.init(Thread.currentThread());
- targetAndModelRealm.block();
- }
- }.start();
+ initRealm(targetRealm, true);
+ initRealm(modelRealm, true);
+ initRealm(validationRealm, false);
+ mainThread.init(Thread.currentThread());
- validationRealm.init(Thread.currentThread());
dbc = new DataBindingContext(validationRealm);
Policy.setLog(logger);
}
+ private void initRealm(final ThreadRealm realm, final boolean block) {
+ new Thread() {
+ @Override
+ public void run() {
+ realm.init(Thread.currentThread());
+ if (block) {
+ realm.block();
+ }
+ }
+ }.start();
+ if (block) {
+ realm.waitUntilBlocking();
+ }
+ }
+
@After
public void tearDown() throws Exception {
- dbc.dispose();
+ validationRealm.exec(() -> dbc.dispose());
}
@Test
public void testListBindingValidationRealm() throws Throwable {
- final ObservableList model = new WritableList(targetAndModelRealm);
- final ObservableList target = new WritableList(targetAndModelRealm);
+ final IObservableList<?> model = new WritableList<>(targetRealm);
+ final IObservableList<?> target = new WritableList<>(targetRealm);
dbc.bindList(target, model);
- targetAndModelRealm.waitUntilBlocking();
- targetAndModelRealm.processQueue();
- targetAndModelRealm.unblock();
+ targetRealm.waitUntilBlocking();
+ targetRealm.processQueue();
+ targetRealm.unblock();
assertTrue(errorStatusses.toString(), errorStatusses.isEmpty());
}
@Test
public void testSetBindingValidationRealm() throws Throwable {
- final ObservableSet model = new WritableSet(targetAndModelRealm);
- final ObservableSet target = new WritableSet(targetAndModelRealm);
+ final IObservableSet<?> model = new WritableSet<>(targetRealm);
+ final IObservableSet<?> target = new WritableSet<>(targetRealm);
dbc.bindSet(target, model);
- targetAndModelRealm.waitUntilBlocking();
- targetAndModelRealm.processQueue();
- targetAndModelRealm.unblock();
+ targetRealm.waitUntilBlocking();
+ targetRealm.processQueue();
+ targetRealm.unblock();
assertTrue(errorStatusses.toString(), errorStatusses.isEmpty());
}
@Test
public void testBindingCanBeCreatedOutsideOfValidationRealm() throws Exception {
- final ObservableSet<String> model = new WritableSet<>(targetAndModelRealm);
- final ObservableSet<String> target = new WritableSet<>(targetAndModelRealm);
+ final ObservableSet<String> model = new WritableSet<>(targetRealm);
+ final ObservableSet<String> target = new WritableSet<>(targetRealm);
- targetAndModelRealm.unblock();
+ targetRealm.unblock();
AtomicReference<Exception> exceptionOccured = new AtomicReference<>();
Thread t = new Thread() {
@@ -127,61 +140,119 @@
@Test
public void testBindComputedListToWritableListInDifferentRealm() {
- // The validationRealm is the current realm.
- final IObservableValue<String> modelValue = new WritableValue<>(validationRealm);
- final IObservableList<String> model = new ComputedList<String>(validationRealm) {
+ final IObservableValue<String> modelValue = new WritableValue<>(mainThread);
+ final IObservableList<String> model = new ComputedList<String>(mainThread) {
@Override
protected List<String> calculate() {
return Collections.singletonList(modelValue.getValue());
}
};
- final IObservableList<String> target = new WritableList<>(targetAndModelRealm);
+ final IObservableList<String> target = new WritableList<>(targetRealm);
dbc.bindList(target, model);
modelValue.setValue("Test");
- targetAndModelRealm.waitUntilBlocking();
- targetAndModelRealm.processQueue();
- targetAndModelRealm.unblock();
+ targetRealm.waitUntilBlocking();
+ targetRealm.processQueue();
+ targetRealm.unblock();
assertTrue(errorStatusses.toString(), errorStatusses.isEmpty());
}
@Test
public void testBindComputedSetToWritableSetInDifferentRealm() {
- // The validationRealm is the current realm.
- final IObservableValue<String> modelValue = new WritableValue<>(validationRealm);
- final IObservableSet<String> model = new ComputedSet<String>(validationRealm) {
+ final IObservableValue<String> modelValue = new WritableValue<>(mainThread);
+ final IObservableSet<String> model = new ComputedSet<String>(mainThread) {
@Override
protected Set<String> calculate() {
return Collections.singleton(modelValue.getValue());
}
};
- final IObservableSet<String> target = new WritableSet<>(targetAndModelRealm);
+ final IObservableSet<String> target = new WritableSet<>(targetRealm);
dbc.bindSet(target, model);
modelValue.setValue("Test");
- targetAndModelRealm.waitUntilBlocking();
- targetAndModelRealm.processQueue();
- targetAndModelRealm.unblock();
+ targetRealm.waitUntilBlocking();
+ targetRealm.processQueue();
+ targetRealm.unblock();
assertTrue(errorStatusses.toString(), errorStatusses.isEmpty());
}
@Test
public void testBindComputedValueToWritableValueInDifferentRealm() {
- // The validationRealm is the current realm.
- final IObservableValue<String> modelValue = new WritableValue<>(validationRealm);
- final IObservableValue<String> model = new ComputedValue<String>(validationRealm) {
+ final IObservableValue<String> modelValue = new WritableValue<>(mainThread);
+ final IObservableValue<String> model = new ComputedValue<String>(mainThread) {
@Override
protected String calculate() {
return modelValue.getValue();
}
};
- final IObservableValue<String> target = new WritableValue<>(targetAndModelRealm);
+ final IObservableValue<String> target = new WritableValue<>(targetRealm);
dbc.bindValue(target, model);
modelValue.setValue("Test");
- targetAndModelRealm.waitUntilBlocking();
- targetAndModelRealm.processQueue();
- targetAndModelRealm.unblock();
+ targetRealm.waitUntilBlocking();
+ targetRealm.processQueue();
+ targetRealm.unblock();
assertTrue(errorStatusses.toString(), errorStatusses.isEmpty());
}
+
+ @Test
+ public void testSetBindingUpdatesDontInterferWithObservableDisposing() throws Exception {
+ final IObservableSet<String> model = new WritableSet<>(modelRealm);
+ final IObservableSet<String> target = new WritableSet<>(targetRealm);
+ dbc.bindSet(target, model);
+
+ waitForBindingInitiated();
+
+ modelRealm.exec(() -> model.add("one"));
+
+ testDisposeRaceCondition(target);
+ }
+
+ @Test
+ public void testListBindingUpdatesDontInterferWithObservableDisposing() throws Exception {
+ final IObservableList<String> model = new WritableList<>(modelRealm);
+ final IObservableList<String> target = new WritableList<>(targetRealm);
+ dbc.bindList(target, model);
+
+ waitForBindingInitiated();
+
+ modelRealm.exec(() -> model.add("one"));
+
+ testDisposeRaceCondition(target);
+ }
+
+ @Test
+ public void testValueBindingUpdatesDontInterferWithObservableDisposing() throws Exception {
+ final IObservableValue<String> model = new WritableValue<>(modelRealm);
+ final IObservableValue<String> target = new WritableValue<>(targetRealm);
+ dbc.bindValue(target, model);
+
+ waitForBindingInitiated();
+
+ modelRealm.exec(() -> model.setValue("one"));
+
+ testDisposeRaceCondition(target);
+ }
+
+ private void testDisposeRaceCondition(final IObservable target) {
+ modelRealm.processQueue();
+ targetRealm.waitUntilBlocking();
+
+ target.dispose();
+
+ modelRealm.unblock();
+ targetRealm.processQueue();
+ targetRealm.unblock();
+ validationRealm.unblock();
+
+ assertTrue(errorStatusses.toString(), errorStatusses.isEmpty());
+ }
+
+ private void waitForBindingInitiated() {
+ modelRealm.waitUntilBlocking();
+ modelRealm.processQueue();
+
+ targetRealm.waitUntilBlocking();
+ targetRealm.processQueue();
+ }
}
