Skip to content

Commit

Permalink
8224260: ChangeListener not triggered when adding a new listener in i…
Browse files Browse the repository at this point in the history 
…nvalidated method

Reviewed-by: kcr, nlisker
  • Loading branch information
@hjohn
hjohn committed Apr 11, 2023
1 parent 5395942 commit fb63b26
Show file tree
Hide file tree
Showing 2 changed files with 117 additions and 26 deletions.
40 changes: 26 additions & 14 deletions modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,23 @@
/**
* A convenience class for creating implementations of {@link javafx.beans.value.ObservableValue}.
* It contains all of the infrastructure support for value invalidation- and
* change event notification.
* change event notification.<p>
*
* This implementation can handle adding and removing listeners while the
* observers are being notified, but it is not thread-safe.
* observers are being notified, but it is not thread-safe.<p>
*
* This class keeps track of the latest value it has seen to determine if change
* listeners should be called when next {@link #fireValueChangedEvent()} is called.
* So while this value is usually the current value of the involved observable,
* it becomes the "old" value as soon as the observable is changed, until such time
* it is updated again (by calling {@link #fireValueChangedEvent()}).<p>
*
* During this brief period, listeners may be added or removed causing the ExpressionHelper
* to perhaps switch to a different variant of itself. These different variants must be
* made aware of the currently stored latest value, as obtaining this value from the
* {@link ObservableValue} would (during that brief period) be a different value. Using
* the incorrect latest value would result in change listeners not being fired as they
* perform an equality check.
*/
public abstract class ExpressionHelper<T> extends ExpressionHelperBase {

Expand All @@ -65,7 +76,7 @@ public static <T> ExpressionHelper<T> addListener(ExpressionHelper<T> helper, Ob
if ((observable == null) || (listener == null)) {
throw new NullPointerException();
}
return (helper == null)? new SingleChange<>(observable, listener) : helper.addListener(listener);
return (helper == null)? new SingleChange<>(observable, observable.getValue(), listener) : helper.addListener(listener);
}

public static <T> ExpressionHelper<T> removeListener(ExpressionHelper<T> helper, ChangeListener<? super T> listener) {
Expand Down Expand Up @@ -122,7 +133,7 @@ protected ExpressionHelper<T> removeListener(InvalidationListener listener) {

@Override
protected ExpressionHelper<T> addListener(ChangeListener<? super T> listener) {
return new Generic<>(observable, this.listener, listener);
return new Generic<>(observable, observable.getValue(), this.listener, listener);
}

@Override
Expand All @@ -145,15 +156,15 @@ private static class SingleChange<T> extends ExpressionHelper<T> {
private final ChangeListener<? super T> listener;
private T currentValue;

private SingleChange(ObservableValue<T> observable, ChangeListener<? super T> listener) {
private SingleChange(ObservableValue<T> observable, T currentValue, ChangeListener<? super T> listener) {
super(observable);
this.listener = listener;
this.currentValue = observable.getValue();
this.currentValue = currentValue;
}

@Override
protected ExpressionHelper<T> addListener(InvalidationListener listener) {
return new Generic<>(observable, listener, this.listener);
return new Generic<>(observable, currentValue, listener, this.listener);
}

@Override
Expand All @@ -163,7 +174,7 @@ protected ExpressionHelper<T> removeListener(InvalidationListener listener) {

@Override
protected ExpressionHelper<T> addListener(ChangeListener<? super T> listener) {
return new Generic<>(observable, this.listener, listener);
return new Generic<>(observable, currentValue, this.listener, listener);
}

@Override
Expand Down Expand Up @@ -201,20 +212,20 @@ private Generic(ObservableValue<T> observable, InvalidationListener listener0, I
this.invalidationSize = 2;
}

private Generic(ObservableValue<T> observable, ChangeListener<? super T> listener0, ChangeListener<? super T> listener1) {
private Generic(ObservableValue<T> observable, T currentValue, ChangeListener<? super T> listener0, ChangeListener<? super T> listener1) {
super(observable);
this.changeListeners = new ChangeListener[] {listener0, listener1};
this.changeSize = 2;
this.currentValue = observable.getValue();
this.currentValue = currentValue;
}

private Generic(ObservableValue<T> observable, InvalidationListener invalidationListener, ChangeListener<? super T> changeListener) {
private Generic(ObservableValue<T> observable, T currentValue, InvalidationListener invalidationListener, ChangeListener<? super T> changeListener) {
super(observable);
this.invalidationListeners = new InvalidationListener[] {invalidationListener};
this.invalidationSize = 1;
this.changeListeners = new ChangeListener[] {changeListener};
this.changeSize = 1;
this.currentValue = observable.getValue();
this.currentValue = currentValue;
}

@Override
Expand Down Expand Up @@ -246,7 +257,7 @@ protected ExpressionHelper<T> removeListener(InvalidationListener listener) {
if (listener.equals(invalidationListeners[index])) {
if (invalidationSize == 1) {
if (changeSize == 1) {
return new SingleChange<>(observable, changeListeners[0]);
return new SingleChange<>(observable, currentValue, changeListeners[0]);
}
invalidationListeners = null;
invalidationSize = 0;
Expand Down Expand Up @@ -310,8 +321,9 @@ protected ExpressionHelper<T> removeListener(ChangeListener<? super T> listener)
}
changeListeners = null;
changeSize = 0;
currentValue = null; // clear current value to avoid stale reference
} else if ((changeSize == 2) && (invalidationSize == 0)) {
return new SingleChange<>(observable, changeListeners[1-index]);
return new SingleChange<>(observable, currentValue, changeListeners[1-index]);
} else {
final int numMoved = changeSize - index - 1;
final ChangeListener<? super T>[] oldListeners = changeListeners;
Expand Down
103 changes: 91 additions & 12 deletions modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ExpressionHelperTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,26 +25,36 @@

package test.com.sun.javafx.binding;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;

import java.util.BitSet;
import java.util.List;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicReference;

import org.junit.Before;
import org.junit.Test;

import com.sun.javafx.binding.ExpressionHelper;
import com.sun.javafx.binding.ExpressionHelperShim;

import javafx.beans.InvalidationListener;
import test.javafx.beans.InvalidationListenerMock;
import javafx.beans.Observable;
import test.javafx.beans.WeakInvalidationListenerMock;
import javafx.beans.property.ObjectProperty;
import javafx.beans.property.SimpleObjectProperty;
import javafx.beans.property.SimpleStringProperty;
import javafx.beans.property.StringProperty;
import javafx.beans.value.ChangeListener;
import test.javafx.beans.value.ChangeListenerMock;
import javafx.beans.value.ObservableValue;
import javafx.beans.value.ObservableValueStub;
import test.javafx.beans.InvalidationListenerMock;
import test.javafx.beans.WeakInvalidationListenerMock;
import test.javafx.beans.value.ChangeListenerMock;
import test.javafx.beans.value.WeakChangeListenerMock;
import org.junit.Before;
import org.junit.Test;

import java.util.BitSet;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import test.util.memory.JMemoryBuddy;

public class ExpressionHelperTest {

Expand Down Expand Up @@ -645,4 +655,73 @@ public void testExceptionHandledByThreadUncaughtHandlerInMultipleChangeAndInvali
assertEquals(4, called.get());
}

@Test
public void shouldNotForgetCurrentValueWhenMovingFromSingleChangeToGenericImplementation() {
AtomicBoolean invalidated = new AtomicBoolean();
AtomicReference<String> currentValue = new AtomicReference<>();
StringProperty p = new SimpleStringProperty("a") {
@Override
protected void invalidated() {
addListener(obs -> invalidated.set(true));
}
};

p.addListener((obs, old, current) -> currentValue.set(current));

p.set("b");

assertTrue(invalidated.get()); // true because the invalidation listener was added before called
assertEquals("b", currentValue.get());
}

@Test
public void shouldNotForgetCurrentValueWhenMovingFromGenericToSingleChangeImplementation() {
AtomicBoolean invalidated = new AtomicBoolean();
AtomicReference<String> currentValue = new AtomicReference<>();
InvalidationListener invalidationListener = obs -> invalidated.set(true);
StringProperty p = new SimpleStringProperty("a") {
@Override
protected void invalidated() {
removeListener(invalidationListener);
}
};

p.addListener(invalidationListener);
p.addListener((obs, old, current) -> currentValue.set(current));

p.set("b");

assertFalse(invalidated.get()); // false because the invalidation listener was removed before called
assertEquals("b", currentValue.get());
}

@Test
public void shouldNotReferToAnOldValueWhenAllChangeListenersRemoved() {
AtomicInteger invalidations = new AtomicInteger();
ObjectProperty<List<String>> p = new SimpleObjectProperty<>(List.of("a"));

// add two invalidation listeners so Generic implementation is used:
p.addListener(obs -> invalidations.addAndGet(1));
p.addListener(obs -> invalidations.addAndGet(1));

// add a change listener:
ChangeListener<? super List<String>> listener = (obs, old, current) -> {};

p.addListener(listener);

JMemoryBuddy.memoryTest(api -> {
// trigger a change to a value that should be collectable:
List<String> collectable = List.of("b");

p.set(collectable);

// remove the last change listener:
p.removeListener(listener);

// change value to something else, it should not be referenced anymore anywhere:
p.set(List.of("c"));

api.assertCollectable(collectable);
});
}
}

0 comments on commit fb63b26

Please sign in to comment.