Fix subtle problems with MemoizationList:

* Properly handle recursion in get() and force(). * Make sure not to access underlying sparse list when memoization is off, because it is not being kept updated then.
TomasMikula · Feb 8, 2015 · f242a67 · f242a67
1 parent 08dd528
commit f242a67
Show file tree

Hide file tree

Showing 4 changed files with 56 additions and 13 deletions.
diff --git a/reactfx/src/main/java/org/reactfx/collection/MemoizationList.java b/reactfx/src/main/java/org/reactfx/collection/MemoizationList.java
@@ -60,7 +60,6 @@ private void publishNotifications() {
 
     MemoizationListImpl(ObservableList<E> source) {
         this.source = source;
-        sparseList.insertVoid(0, source.size());
     }
 
     @Override
@@ -87,12 +86,14 @@ private void sourceChanged(QuasiListChange<? extends E> qc) {
 
     @Override
     public E get(int index) {
-        if(sparseList.isPresent(index)) {
+        if(!isObservingInputs()) { // memoization is off
+            return source.get(index);
+        } else if(sparseList.isPresent(index)) {
             return sparseList.getOrThrow(index);
         } else {
-            E elem = source.get(index);
-            if(isObservingInputs()) { // memoization is on
-                sparseList.set(index, elem);
+            E elem = source.get(index); // may cause recursive get(), so we
+                                        // need to check again for absence
+            if(sparseList.setIfAbsent(index, elem)) {
                 memoizedItems.fireElemInsertion(
                         sparseList.getPresentCountBefore(index));
             }
@@ -115,8 +116,9 @@ public void force(int from, int to) {
         for(int i = from; i < to; ++i) {
             if(!sparseList.isPresent(i)) {
                 E elem = source.get(i);
-                sparseList.set(i, elem);
-                mods.add(LiveListHelpers.elemInsertion(presentBefore + (i - from)));
+                if(sparseList.setIfAbsent(i, elem)) {
+                    mods.add(LiveListHelpers.elemInsertion(presentBefore + (i - from)));
+                }
             }
         }
         if(!mods.isEmpty()) {
@@ -136,22 +138,31 @@ public LiveList<E> memoizedItems() {
 
     @Override
     public boolean isMemoized(int index) {
-        return sparseList.isPresent(index);
+        return isObservingInputs() && sparseList.isPresent(index);
     }
 
     @Override
     public Optional<E> getIfMemoized(int index) {
-        return sparseList.get(index);
+        Lists.checkIndex(index, size());
+        return isObservingInputs()
+                ? sparseList.get(index)
+                : Optional.empty();
     }
 
     @Override
     public int getMemoizedCountBefore(int position) {
-        return sparseList.getPresentCountBefore(position);
+        Lists.checkPosition(position, size());
+        return isObservingInputs()
+                ? sparseList.getPresentCountBefore(position)
+                : 0;
     }
 
     @Override
     public int getMemoizedCountAfter(int position) {
-        return sparseList.getPresentCountAfter(position);
+        Lists.checkPosition(position, size());
+        return isObservingInputs()
+                ? sparseList.getPresentCountAfter(position)
+                : 0;
     }
 
     @Override

diff --git a/reactfx/src/main/java/org/reactfx/util/SparseList.java b/reactfx/src/main/java/org/reactfx/util/SparseList.java
@@ -348,6 +348,15 @@ public void set(int index, E elem) {
         });
     }
 
+    public boolean setIfAbsent(int index, E elem) {
+        if(isPresent(index)) {
+            return false;
+        } else {
+            set(index, elem);
+            return true;
+        }
+    }
+
     public void insert(int position, E elem) {
         insertAll(position, Collections.singleton(elem));
     }

diff --git a/reactfx/src/main/java/org/reactfx/value/Val.java b/reactfx/src/main/java/org/reactfx/value/Val.java
@@ -73,8 +73,7 @@ default void removeListener(ChangeListener<? super T> listener) {
 
     /**
      * Adds a change listener and returns a Subscription that can be
-     * used to remove that listener. See the example at
-     * {@link #observeInvalidations(InvalidationListener)}.
+     * used to remove that listener.
      */
     default Subscription observeChanges(ChangeListener<? super T> listener) {
         return observeInvalidations(new ChangeListenerWrapper<>(this, listener));

diff --git a/reactfx/src/test/java/org/reactfx/collection/MemoizationListTest.java b/reactfx/src/test/java/org/reactfx/collection/MemoizationListTest.java
@@ -6,6 +6,7 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
+import java.util.function.Function;
 
 import javafx.beans.property.IntegerProperty;
 import javafx.beans.property.SimpleIntegerProperty;
@@ -165,4 +166,27 @@ public void testForgetIsNotAllowedWhenUnobserved() {
 
         list.forget(2, 4);
     }
+
+    @Test
+    public void testRecursionWithinForce() {
+        LiveList<Integer> src = new LiveArrayList<>(0, 1, 2);
+        MemoizationList<Integer> memo1 = src.memoize();
+        MemoizationList<Integer> memo2 = memo1.map(Function.identity()).memoize();
+        memo1.memoizedItems().sizeProperty().observeInvalidations(__ -> memo2.force(1, 2));
+
+        List<Integer> memo2Mirror = new ArrayList<>();
+        memo2.memoizedItems().observeModifications(mod -> {
+            memo2Mirror.subList(mod.getFrom(), mod.getFrom() + mod.getRemovedSize()).clear();
+            memo2Mirror.addAll(mod.getFrom(), mod.getAddedSubList());
+        });
+
+        assertEquals(Collections.emptyList(), memo1.memoizedItems());
+        assertEquals(Collections.emptyList(), memo2.memoizedItems());
+        assertEquals(Collections.emptyList(), memo2Mirror);
+
+        memo2.force(1, 2); // causes an immediate change in memo1.memoizedItems(),
+                           // which recursively calls memo2.force(1, 2).
+
+        assertEquals(Arrays.asList(1), memo2Mirror);
+    }
 }