Update NodeMap#setContent to avoid clear (#4284)

common/src/main/java/me/lucko/luckperms/common/model/nodemap/NodeMap.java

@@ -151,8 +151,6 @@ public interface NodeMap {
 
     Difference<Node> setContent(Iterable<? extends Node> set);
 
-    Difference<Node> setContent(Stream<? extends Node> stream);
-
     Difference<Node> applyChanges(Difference<Node> changes);
 
     Difference<Node> addAll(Iterable<? extends Node> set);

common/src/main/java/me/lucko/luckperms/common/model/nodemap/NodeMapMutable.java

@@ -71,21 +71,19 @@ public class NodeMapMutable extends NodeMapBase {
      * The node values are ordered according to the priority rules defined in NodeComparator.
      *
      * We use our own "multimap"-like implementation here because guava's is not thread safe.
-     *
-     * The map fields aren't final because they are replaced when large updates (e.g. clear)
-     * are performed. We do this so there's no risk that the read methods will see an inconsistent
-     * state in the middle of an update from the DB. (see below comment about locking - we don't
-     * lock for reads!)
      */
-    private SortedMap<ImmutableContextSet, SortedSet<Node>> map = createMap();
-    private SortedMap<ImmutableContextSet, SortedSet<InheritanceNode>> inheritanceMap = createMap();
+    private final SortedMap<ImmutableContextSet, SortedSet<Node>> map = createMap();
+    private final SortedMap<ImmutableContextSet, SortedSet<InheritanceNode>> inheritanceMap = createMap();
 
     /**
-     * This lock is used whilst performing mutations, but *not* reads.
+     * This lock is used whilst performing writes, but *not* reads.
      *
      * The maps themselves are thread safe, so for querying, we just allow
      * the read methods to do whatever they want without any locking.
-     * However, we want mutations to be atomic, so we use the lock to ensure that happens.
+     *
+     * Readers can see partially inconsistent data, but this is fine - the data is only ever used for
+     * querying, and the worst case scenario is that a query returns a node that has just been removed,
+     * or doesn't return a node that has just been added.
      */
     private final Lock lock = new ReentrantLock();
 
@@ -324,10 +322,8 @@ public class NodeMapMutable extends NodeMapBase {
                 result.recordChanges(ChangeType.REMOVE, nodes);
             }
 
-            // replace the map - this means any client reading async won't be affected
-            // by any race conditions between this call to clear and any subsequent call to setContent
-            this.map = createMap();
-            this.inheritanceMap = createMap();
+            this.map.clear();
+            this.inheritanceMap.clear();
         } finally {
             this.lock.unlock();
         }
@@ -356,32 +352,18 @@ public class NodeMapMutable extends NodeMapBase {
 
     @Override
     public Difference<Node> setContent(Iterable<? extends Node> set) {
-        Difference<Node> result = new Difference<>();
+        Difference<Node> diff = new Difference<>();
+        diff.recordChanges(ChangeType.ADD, set);
 
         this.lock.lock();
         try {
-            result.mergeFrom(clear());
-            result.mergeFrom(addAll(set));
+            for (SortedSet<Node> nodes : this.map.values()) {
+                diff.recordChanges(ChangeType.REMOVE, nodes);
+            }
+            return applyChanges(diff);
         } finally {
             this.lock.unlock();
         }
-
-        return result;
-    }
-
-    @Override
-    public Difference<Node> setContent(Stream<? extends Node> stream) {
-        Difference<Node> result = new Difference<>();
-
-        this.lock.lock();
-        try {
-            result.mergeFrom(clear());
-            result.mergeFrom(addAll(stream));
-        } finally {
-            this.lock.unlock();
-        }
-
-        return result;
     }
 
     @Override

common/src/main/java/me/lucko/luckperms/common/model/nodemap/RecordedNodeMap.java

@@ -45,6 +45,7 @@ import java.util.SortedSet;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
 import java.util.function.Consumer;
+import java.util.function.Function;
 import java.util.function.Predicate;
 import java.util.stream.Stream;
 
@@ -91,84 +92,80 @@ public class RecordedNodeMap implements NodeMap {
     public Difference<Node> addDefaultNodeToChangeSet() {
         Difference<Node> diff = new Difference<>();
         diff.recordChange(Difference.ChangeType.ADD, Inheritance.builder(GroupManager.DEFAULT_GROUP_NAME).build());
-        return record(diff);
+        return record(map -> diff);
     }
 
-    private Difference<Node> record(Difference<Node> result) {
+    private Difference<Node> record(Function<NodeMap, Difference<Node>> func) {
         this.lock.lock();
         try {
+            Difference<Node> result = func.apply(this.delegate);
             this.changes.mergeFrom(result);
+            return result;
         } finally {
             this.lock.unlock();
         }
-        return result;
     }
 
     // delegate, but pass the result through #record(MutateResult)
     
     @Override
     public Difference<Node> add(Node nodeWithoutInheritanceOrigin) {
-        return record(this.delegate.add(nodeWithoutInheritanceOrigin));
+        return record(map -> map.add(nodeWithoutInheritanceOrigin));
     }
 
     @Override
     public Difference<Node> remove(Node node) {
-        return record(this.delegate.remove(node));
+        return record(map -> map.remove(node));
     }
 
     @Override
     public Difference<Node> removeExact(Node node) {
-        return record(this.delegate.removeExact(node));
+        return record(map -> map.removeExact(node));
     }
 
     @Override
     public Difference<Node> removeIf(Predicate<? super Node> predicate) {
-        return record(this.delegate.removeIf(predicate));
+        return record(map -> map.removeIf(predicate));
     }
 
     @Override
     public Difference<Node> removeIf(ContextSet contextSet, Predicate<? super Node> predicate) {
-        return record(this.delegate.removeIf(contextSet, predicate));
+        return record(map -> map.removeIf(contextSet, predicate));
     }
 
     @Override
     public Difference<Node> removeThenAdd(Node nodeToRemove, Node nodeToAdd) {
-        return record(this.delegate.removeThenAdd(nodeToRemove, nodeToAdd));
+        return record(map -> map.removeThenAdd(nodeToRemove, nodeToAdd));
     }
 
     @Override
     public Difference<Node> clear() {
-        return record(this.delegate.clear());
+        return record(map -> map.clear());
     }
 
     @Override
     public Difference<Node> clear(ContextSet contextSet) {
-        return record(this.delegate.clear(contextSet));
+        return record(map -> map.clear(contextSet));
     }
 
     @Override
     public Difference<Node> setContent(Iterable<? extends Node> set) {
-        return record(this.delegate.setContent(set));
-    }
-
-    @Override
-    public Difference<Node> setContent(Stream<? extends Node> stream) {
-        return record(this.delegate.setContent(stream));
+        return record(map -> map.setContent(set));
     }
 
     @Override
     public Difference<Node> applyChanges(Difference<Node> changes) {
-        return record(this.delegate.applyChanges(changes));
+        return record(map -> map.applyChanges(changes));
     }
 
     @Override
     public Difference<Node> addAll(Iterable<? extends Node> set) {
-        return record(this.delegate.addAll(set));
+        return record(map -> map.addAll(set));
     }
 
     @Override
     public Difference<Node> addAll(Stream<? extends Node> stream) {
-        return record(this.delegate.addAll(stream));
+        return record(map -> map.addAll(stream));
     }
 
     // just plain delegation

common/src/main/java/me/lucko/luckperms/common/util/Difference.java

@@ -26,7 +26,6 @@
 package me.lucko.luckperms.common.util;
 
 import java.util.LinkedHashSet;
-import java.util.Objects;
 import java.util.Set;
 import java.util.stream.Stream;
 
@@ -98,18 +97,6 @@ public class Difference<T> {
         this.changes.clear();
     }
 
-    private void recordChange(Change<T> change) {
-        // This method is the magic of this class.
-        // When tracking, we want to ignore changes that cancel each other out, and only
-        // keep track of the net difference.
-        // e.g. adding then removing the same value = zero net change, so ignore it.
-
-        if (this.changes.remove(change.inverse())) {
-            return;
-        }
-        this.changes.add(change);
-    }
-
     /**
      * Records a change.
      *
@@ -117,7 +104,14 @@ public class Difference<T> {
      * @param value the changed value
      */
     public void recordChange(ChangeType type, T value) {
-        recordChange(new Change<>(type, value));
+        // This method is the magic of this class.
+        // When tracking, we want to ignore changes that cancel each other out, and only
+        // keep track of the net difference.
+        // e.g. adding then removing the same value = zero net change, so ignore it.
+        if (this.changes.remove(new Change<>(type.inverse(), value))) {
+            return;
+        }
+        this.changes.add(new Change<>(type, value));
     }
 
     /**
@@ -128,20 +122,10 @@ public class Difference<T> {
      */
     public void recordChanges(ChangeType type, Iterable<? extends T> values) {
         for (T value : values) {
-            recordChange(new Change<>(type, value));
+            recordChange(type, value);
         }
     }
 
-    /**
-     * Records some changes.
-     *
-     * @param type the type of change
-     * @param values the changed values
-     */
-    public void recordChanges(ChangeType type, Stream<? extends T> values) {
-        values.forEach(value -> recordChange(new Change<>(type, value)));
-    }
-
     /**
      * Merges the recorded differences in {@code other} into this.
      *
@@ -150,7 +134,7 @@ public class Difference<T> {
      */
     public Difference<T> mergeFrom(Difference<T> other) {
         for (Change<T> change : other.changes) {
-            recordChange(change);
+            recordChange(change.type(), change.value());
         }
         return this;
     }
@@ -168,10 +152,12 @@ public class Difference<T> {
     public static final class Change<T> {
         private final ChangeType type;
         private final T value;
+        private final int hashCode;
 
         public Change(ChangeType type, T value) {
             this.type = type;
             this.value = value;
+            this.hashCode = 31 * type.hashCode() + value.hashCode();
         }
 
         public ChangeType type() {
@@ -182,10 +168,6 @@ public class Difference<T> {
             return this.value;
         }
 
-        public Change<T> inverse() {
-            return new Change<>(this.type.inverse(), this.value);
-        }
-
         @Override
         public boolean equals(Object o) {
             if (this == o) return true;
@@ -196,7 +178,7 @@ public class Difference<T> {
 
         @Override
         public int hashCode() {
-            return Objects.hash(this.type, this.value);
+            return this.hashCode;
         }
 
         @Override