Ensure cached document tree listeners are run after cache has been updated to prevent stale reads from the cache
Change-Id: I6cad61bd0fcec15b96cc7e418f5ec2471ee62930
diff --git a/core/store/primitives/src/main/java/org/onosproject/store/primitives/impl/CachingAsyncDocumentTree.java b/core/store/primitives/src/main/java/org/onosproject/store/primitives/impl/CachingAsyncDocumentTree.java
index d71f965..eece91e 100644
--- a/core/store/primitives/src/main/java/org/onosproject/store/primitives/impl/CachingAsyncDocumentTree.java
+++ b/core/store/primitives/src/main/java/org/onosproject/store/primitives/impl/CachingAsyncDocumentTree.java
@@ -15,14 +15,17 @@
*/
package org.onosproject.store.primitives.impl;
+import java.util.Map;
import java.util.concurrent.CompletableFuture;
import java.util.function.Consumer;
import com.google.common.cache.CacheBuilder;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
+import com.google.common.collect.Maps;
import org.onosproject.store.service.AsyncDocumentTree;
import org.onosproject.store.service.DocumentPath;
+import org.onosproject.store.service.DocumentTreeEvent;
import org.onosproject.store.service.DocumentTreeListener;
import org.onosproject.store.service.Versioned;
import org.slf4j.Logger;
@@ -38,6 +41,7 @@
private static final int DEFAULT_CACHE_SIZE = 10000;
private final Logger log = getLogger(getClass());
+ private final Map<DocumentTreeListener<V>, DocumentTreeListener<V>> eventListeners = Maps.newConcurrentMap();
private final LoadingCache<DocumentPath, CompletableFuture<Versioned<V>>> cache;
private final DocumentTreeListener<V> cacheUpdater;
private final Consumer<Status> statusListener;
@@ -68,6 +72,7 @@
} else {
cache.put(event.path(), CompletableFuture.completedFuture(event.newValue().get()));
}
+ eventListeners.values().forEach(listener -> listener.event(event));
};
statusListener = status -> {
log.debug("{} status changed to {}", this.name(), status);
@@ -77,7 +82,7 @@
cache.invalidateAll();
}
};
- super.addListener(cacheUpdater);
+ super.addListener(root(), cacheUpdater);
super.addStatusChangeListener(statusListener);
}
@@ -129,4 +134,33 @@
return super.removeNode(path)
.whenComplete((r, e) -> cache.invalidate(path));
}
+
+ @Override
+ public CompletableFuture<Void> addListener(DocumentPath path, DocumentTreeListener<V> listener) {
+ eventListeners.put(listener, new InternalListener(path, listener));
+ return CompletableFuture.completedFuture(null);
+ }
+
+ @Override
+ public CompletableFuture<Void> removeListener(DocumentTreeListener<V> listener) {
+ eventListeners.remove(listener);
+ return CompletableFuture.completedFuture(null);
+ }
+
+ private class InternalListener implements DocumentTreeListener<V> {
+ private final DocumentPath path;
+ private final DocumentTreeListener<V> listener;
+
+ public InternalListener(DocumentPath path, DocumentTreeListener<V> listener) {
+ this.path = path;
+ this.listener = listener;
+ }
+
+ @Override
+ public void event(DocumentTreeEvent<V> event) {
+ if (event.path().isDescendentOf(path)) {
+ listener.event(event);
+ }
+ }
+ }
}
diff --git a/core/store/primitives/src/main/java/org/onosproject/store/primitives/resources/impl/AtomixDocumentTree.java b/core/store/primitives/src/main/java/org/onosproject/store/primitives/resources/impl/AtomixDocumentTree.java
index eadaa7e..877d3fe 100644
--- a/core/store/primitives/src/main/java/org/onosproject/store/primitives/resources/impl/AtomixDocumentTree.java
+++ b/core/store/primitives/src/main/java/org/onosproject/store/primitives/resources/impl/AtomixDocumentTree.java
@@ -16,11 +16,11 @@
package org.onosproject.store.primitives.resources.impl;
-import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.Executor;
import com.google.common.util.concurrent.MoreExecutors;
@@ -79,7 +79,7 @@
.register(AtomixDocumentTreeEvents.NAMESPACE)
.build());
- private final Map<DocumentTreeListener<byte[]>, InternalListener> eventListeners = new HashMap<>();
+ private final Map<DocumentTreeListener<byte[]>, InternalListener> eventListeners = new ConcurrentHashMap<>();
public AtomixDocumentTree(RaftProxy proxy) {
super(proxy);
diff --git a/core/store/primitives/src/test/java/org/onosproject/store/primitives/impl/CachingAsyncDocumentTreeTest.java b/core/store/primitives/src/test/java/org/onosproject/store/primitives/impl/CachingAsyncDocumentTreeTest.java
new file mode 100644
index 0000000..0606e23
--- /dev/null
+++ b/core/store/primitives/src/test/java/org/onosproject/store/primitives/impl/CachingAsyncDocumentTreeTest.java
@@ -0,0 +1,101 @@
+/*
+ * Copyright 2018-present Open Networking Foundation
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.onosproject.store.primitives.impl;
+
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.Executor;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import io.atomix.protocols.raft.proxy.RaftProxy;
+import io.atomix.protocols.raft.service.RaftService;
+import org.junit.Test;
+import org.onosproject.store.primitives.DefaultDocumentTree;
+import org.onosproject.store.primitives.resources.impl.AtomixDocumentTree;
+import org.onosproject.store.primitives.resources.impl.AtomixDocumentTreeService;
+import org.onosproject.store.primitives.resources.impl.AtomixTestBase;
+import org.onosproject.store.serializers.KryoNamespaces;
+import org.onosproject.store.service.DocumentPath;
+import org.onosproject.store.service.DocumentTree;
+import org.onosproject.store.service.Ordering;
+import org.onosproject.store.service.Serializer;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+/**
+ * Unit tests for cached {@link AtomixDocumentTree}.
+ */
+public class CachingAsyncDocumentTreeTest extends AtomixTestBase<AtomixDocumentTree> {
+
+ @Override
+ protected RaftService createService() {
+ return new AtomixDocumentTreeService(Ordering.NATURAL);
+ }
+
+ @Override
+ protected AtomixDocumentTree createPrimitive(RaftProxy proxy) {
+ return new AtomixDocumentTree(proxy);
+ }
+
+ /**
+ * Tests that reads following events are not stale when cached.
+ */
+ @Test
+ public void testCacheConsistency() throws Throwable {
+ Serializer serializer = Serializer.using(KryoNamespaces.BASIC);
+
+ DocumentTree<String> tree1 = new DefaultDocumentTree<>(
+ new CachingAsyncDocumentTree<>(
+ new DefaultDistributedDocumentTree<>(
+ "testCacheConsistency",
+ newPrimitive("testCacheConsistency"),
+ serializer)), 5000);
+ DocumentTree<String> tree2 = new DefaultDocumentTree<>(
+ new CachingAsyncDocumentTree<>(
+ new DefaultDistributedDocumentTree<>(
+ "testCacheConsistency",
+ newPrimitive("testCacheConsistency"),
+ serializer)), 5000);
+
+ CountDownLatch latch = new CountDownLatch(1);
+ AtomicBoolean failed = new AtomicBoolean();
+
+ Executor executor = Executors.newSingleThreadExecutor();
+ DocumentPath path = DocumentPath.from("root|foo");
+ tree1.addListener(path, event -> executor.execute(() -> {
+ // Check only the "baz" value since it's the last one written. If we check for "bar" on the "bar" event,
+ // there's a race in the test wherein the cache can legitimately be updated to "baz" before the next read.
+ if (event.newValue().get().value().equals("baz")) {
+ try {
+ assertEquals(event.newValue().get().value(), tree1.get(path).value());
+ } catch (AssertionError e) {
+ failed.set(true);
+ }
+ latch.countDown();
+ }
+ }));
+
+ tree2.set(path, "bar");
+ tree2.set(path, "baz");
+
+ latch.await(10, TimeUnit.SECONDS);
+ if (latch.getCount() == 1 || failed.get()) {
+ fail();
+ }
+ }
+}
\ No newline at end of file