(draf): ChangeListener events are incorrect or misleading when a nested change occurs

modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java

@@ -25,12 +25,14 @@
 
 package com.sun.javafx.binding;
 
+import java.util.ArrayDeque;
+import java.util.Arrays;
+import java.util.Deque;
+
 import javafx.beans.InvalidationListener;
 import javafx.beans.value.ChangeListener;
 import javafx.beans.value.ObservableValue;
 
-import java.util.Arrays;
-
 /**
  * A convenience class for creating implementations of {@link javafx.beans.value.ObservableValue}.
  * It contains all of the infrastructure support for value invalidation- and
@@ -43,6 +45,8 @@
  */
 public abstract class ExpressionHelper<T> extends ExpressionHelperBase {
 
+    private static record NestedChange<T>(T value) {}
+
     ////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
     // Static methods
 
@@ -135,7 +139,7 @@ protected void fireValueChangedEvent() {
             try {
                 listener.invalidated(observable);
             } catch (Exception e) {
-                Thread.currentThread().getUncaughtExceptionHandler().uncaughtException(Thread.currentThread(), e);
+                forwardToUncaughtExceptionHandler(e);
             }
         }
     }
@@ -145,6 +149,18 @@ private static class SingleChange<T> extends ExpressionHelper<T> {
         private final ChangeListener<? super T> listener;
         private T currentValue;
 
+        /**
+         * {@code true} when an emission is currently in progress. When
+         * an emission is in progress, nested changes must be queued.
+         */
+        private boolean emissionInProgress;
+
+        /**
+         * When not {@code null}, contains nested changes that will be
+         * emitted in order once the current emission finishes.
+         */
+        private Deque<NestedChange<T>> nestedChanges;
+
         private SingleChange(ObservableValue<T> observable, ChangeListener<? super T> listener) {
             super(observable);
             this.listener = listener;
@@ -171,18 +187,51 @@ protected ExpressionHelper<T> removeListener(ChangeListener<? super T> listener)
             return (listener.equals(this.listener))? null : this;
         }
 
+        private void addNotifyValue() {
+            if (nestedChanges == null) {
+                nestedChanges = new ArrayDeque<>();
+            }
+
+            nestedChanges.add(new NestedChange<>(observable.getValue()));
+        }
+
+        private T nextNotifyValue() {
+            T value = nestedChanges == null ? observable.getValue() : nestedChanges.removeFirst().value;
+
+            if (nestedChanges != null && nestedChanges.isEmpty()) {
+                nestedChanges = null;
+            }
+
+            return value;
+        }
+
         @Override
         protected void fireValueChangedEvent() {
-            final T oldValue = currentValue;
-            currentValue = observable.getValue();
-            final boolean changed = (currentValue == null)? (oldValue != null) : !currentValue.equals(oldValue);
-            if (changed) {
-                try {
-                    listener.changed(observable, oldValue, currentValue);
-                } catch (Exception e) {
-                    Thread.currentThread().getUncaughtExceptionHandler().uncaughtException(Thread.currentThread(), e);
-                }
+            if (emissionInProgress) {
+                addNotifyValue();
+
+                return;
             }
+
+            emissionInProgress = true;
+
+            do {
+                T oldValue = currentValue;
+
+                currentValue = nextNotifyValue();
+
+                boolean changed = (currentValue == null) ? (oldValue != null) : !currentValue.equals(oldValue);
+
+                if (changed) {
+                    try {
+                        listener.changed(observable, oldValue, currentValue);
+                    } catch (Exception e) {
+                        forwardToUncaughtExceptionHandler(e);
+                    }
+                }
+            } while (nestedChanges != null);
+
+            emissionInProgress = false;
         }
     }
 
@@ -192,9 +241,20 @@ private static class Generic<T> extends ExpressionHelper<T> {
         private ChangeListener<? super T>[] changeListeners;
         private int invalidationSize;
         private int changeSize;
-        private boolean locked;
         private T currentValue;
 
+        /**
+         * {@code true} when an emission is currently in progress. When
+         * an emission is in progress, nested changes must be queued.
+         */
+        private boolean locked;
+
+        /**
+         * When not {@code null}, contains nested changes that will be
+         * emitted in order once the current emission finishes.
+         */
+        private Deque<NestedChange<T>> nestedChanges;
+
         private Generic(ObservableValue<T> observable, InvalidationListener listener0, InvalidationListener listener1) {
             super(observable);
             this.invalidationListeners = new InvalidationListener[] {listener0, listener1};
@@ -334,40 +394,90 @@ protected ExpressionHelper<T> removeListener(ChangeListener<? super T> listener)
             return this;
         }
 
+        private void addNotifyValue() {
+            if (nestedChanges == null) {
+                nestedChanges = new ArrayDeque<>();
+            }
+
+            nestedChanges.add(new NestedChange<>(observable.getValue()));
+        }
+
+        private T nextNotifyValue() {
+            T value = nestedChanges == null ? observable.getValue() : nestedChanges.removeFirst().value;
+
+            if (nestedChanges != null && nestedChanges.isEmpty()) {
+                nestedChanges = null;
+            }
+
+            return value;
+        }
+
         @Override
         protected void fireValueChangedEvent() {
+            if (locked) {
+                if (changeSize > 0) {
+                    addNotifyValue();
+                }
+
+                return;
+            }
+
+            locked = true;
+
             final InvalidationListener[] curInvalidationList = invalidationListeners;
             final int curInvalidationSize = invalidationSize;
             final ChangeListener<? super T>[] curChangeList = changeListeners;
             final int curChangeSize = changeSize;
 
-            try {
-                locked = true;
+            do {
+                T oldValue = currentValue;
+
                 for (int i = 0; i < curInvalidationSize; i++) {
                     try {
                         curInvalidationList[i].invalidated(observable);
                     } catch (Exception e) {
-                        Thread.currentThread().getUncaughtExceptionHandler().uncaughtException(Thread.currentThread(), e);
+                        forwardToUncaughtExceptionHandler(e);
                     }
                 }
                 if (curChangeSize > 0) {
-                    final T oldValue = currentValue;
-                    currentValue = observable.getValue();
-                    final boolean changed = (currentValue == null)? (oldValue != null) : !currentValue.equals(oldValue);
+                    currentValue = nextNotifyValue();
+
+                    boolean changed = (currentValue == null) ? (oldValue != null) : !currentValue.equals(oldValue);
+
                     if (changed) {
                         for (int i = 0; i < curChangeSize; i++) {
                             try {
                                 curChangeList[i].changed(observable, oldValue, currentValue);
                             } catch (Exception e) {
-                                Thread.currentThread().getUncaughtExceptionHandler().uncaughtException(Thread.currentThread(), e);
+                                forwardToUncaughtExceptionHandler(e);
                             }
                         }
                     }
                 }
-            } finally {
-                locked = false;
-            }
+            } while(nestedChanges != null);
+
+            locked = false;
         }
     }
 
+    private static void forwardToUncaughtExceptionHandler(Exception e) {
+        try {
+            Thread.currentThread().getUncaughtExceptionHandler().uncaughtException(Thread.currentThread(), e);
+        }
+        catch (Exception ignored) {
+
+            /*
+             * Very bad practice to throw an exception from the uncaught exception handler.
+             * When the JVM calls it, the exception is ignored, however when called directly
+             * this is not the case. To ensure that this class performs its logic correctly,
+             * and doesn't send out partial emissions or gets into an undefined state,
+             * this exception is only logged.
+             */
+
+            System.err.println(
+                "The uncaught exception handler: " + Thread.currentThread().getUncaughtExceptionHandler()
+                    + " threw an exception: " + ignored + " while handling the exception: " + e
+            );
+        }
+    }
 }

modules/javafx.base/src/main/java/javafx/beans/value/ChangeListener.java

@@ -48,8 +48,13 @@ public interface ChangeListener<T> {
     /**
      * Called when the value of an {@link ObservableValue} changes.
      * <p>
-     * In general, it is considered bad practice to modify the observed value in
-     * this method.
+     * Changing the observed value in this method will result in all listeners
+     * being notified of this latest change after the initial change
+     * notification (with the original old and values) has completed.
+     * The listeners that still needed to be notified may see a new value that
+     * differs from a call to {@link ObservableValue#getValue}. All listeners are
+     * then notified again with an old value equal to the initial new value,
+     * and a new value with the latest value.
      *
      * @param observable
      *            The {@code ObservableValue} which value changed

modules/javafx.base/src/test/java/test/com/sun/javafx/binding/ExpressionHelperTest.java

@@ -39,7 +39,9 @@
 import org.junit.Before;
 import org.junit.Test;
 
+import java.util.ArrayList;
 import java.util.BitSet;
+import java.util.List;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
 
@@ -449,6 +451,66 @@ public void testAddChangeWhileLocked() {
         changeListener[3].check(observable, DATA_1, DATA_2, 1);
     }
 
+    @Test
+    public void testChangeValueWhenLocked() {
+        List<String> recording1 = new ArrayList<>();
+        List<String> recording2 = new ArrayList<>();
+        List<String> recording3 = new ArrayList<>();
+
+        final ChangeListener<Integer> recordingChangeListener1 = new ChangeListener<>() {
+            @Override
+            public void changed(ObservableValue<? extends Integer> observable, Integer oldValue, Integer newValue) {
+                recording1.add(oldValue + " -> " + newValue);
+            }
+        };
+
+        final ChangeListener<Integer> recordingChangeListener2 = new ChangeListener<>() {
+            @Override
+            public void changed(ObservableValue<? extends Integer> observable, Integer oldValue, Integer newValue) {
+                recording2.add(oldValue + " -> " + newValue);
+            }
+        };
+
+        final ChangeListener<Integer> changingListener = new ChangeListener<>() {
+            @Override
+            public void changed(ObservableValue<? extends Integer> observable, Integer oldValue, Integer newValue) {
+                recording3.add(oldValue + " -> " + newValue);
+                if(newValue % 2 != 0) {
+                    ExpressionHelperTest.this.observable.set(newValue + 1);
+                    ExpressionHelper.fireValueChangedEvent(helper);
+                }
+            }
+        };
+
+        observable.set(1);
+        ExpressionHelper.fireValueChangedEvent(helper);  // doing this before adding any listeners avoids an exception being logged
+
+        helper = ExpressionHelper.addListener(helper, observable, recordingChangeListener1);
+        helper = ExpressionHelper.addListener(helper, observable, changingListener);
+        helper = ExpressionHelper.addListener(helper, observable, recordingChangeListener2);
+
+        observable.set(2);
+        ExpressionHelper.fireValueChangedEvent(helper);
+
+        assertEquals("1 -> 2", recording1.get(0));
+        assertEquals("1 -> 2", recording2.get(0));
+        assertEquals(1, recording1.size());
+        assertEquals(1, recording2.size());
+
+        recording1.clear();
+        recording2.clear();
+
+        observable.set(3);
+        ExpressionHelper.fireValueChangedEvent(helper);
+
+        assertEquals("2 -> 3", recording1.get(0));
+        assertEquals("2 -> 3", recording2.get(0));
+        assertEquals("3 -> 4", recording1.get(1));
+        assertEquals("3 -> 4", recording2.get(1));
+        assertEquals(2, recording1.size());
+        assertEquals(2, recording2.size());
+    }
+
     @Test
     public void testRemoveChangeWhileLocked() {
         final InvalidationListener removingListener = new InvalidationListener() {