Merge pull request #6031 from vespa-engine/balder/ensure-we-only-create-one-new-cache

Ensure we do not race on cache creation and create many caches that w…

1 files changed, 36 insertions, 6 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabase.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabase.java
index 12a0496ed2e..203f52a7b70 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabase.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabase.java
@@ -38,6 +38,8 @@ public class CuratorDatabase {
     /** Whether we should return data from the cache or always read fro ZooKeeper */
     private final boolean useCache;
 
+    private final Object cacheCreationLock = new Object();
+
     /**
      * All keys, to allow reentrancy.
      * This will grow forever with the number of applications seen, but this should be too slow to be a problem.
@@ -94,14 +96,42 @@ public class CuratorDatabase {
 
     public Optional<byte[]> getData(Path path) { return getCache().getData(path); }
 
+    private static class CacheAndGeneration {
+        public CacheAndGeneration(CuratorDatabaseCache cache, long generation)
+        {
+            this.cache = cache;
+            this.generation = generation;
+        }
+        public boolean expired() {
+            return generation != cache.generation();
+        }
+        public CuratorDatabaseCache validCache() {
+            if (expired()) {
+                throw new IllegalStateException("The cache has generation " + cache.generation() +
+                        " while the root genration counter in zookeeper says " + generation +
+                        ". That is totally unacceptable and must be a sever programming error in my close vicinity.");
+            }
+            return cache;
+        }
+
+        private CuratorDatabaseCache cache;
+        private long generation;
+    }
+    private CacheAndGeneration getCacheSnapshot() {
+        return new CacheAndGeneration(cache.get(), changeGenerationCounter.get());
+    }
     private CuratorDatabaseCache getCache() {
-        CuratorDatabaseCache cache = this.cache.get();
-        long currentCuratorGeneration = changeGenerationCounter.get();
-        if (currentCuratorGeneration != cache.generation()) { // current cache is invalid - start new
-            cache = newCache(currentCuratorGeneration);
-            this.cache.set(cache);
+        CacheAndGeneration cacheAndGeneration = getCacheSnapshot();
+        while (cacheAndGeneration.expired()) {
+            synchronized (cacheCreationLock) { // Prevent a race for creating new caches
+                cacheAndGeneration = getCacheSnapshot();
+                if (cacheAndGeneration.expired()) {
+                    cache.set(newCache(changeGenerationCounter.get()));
+                    cacheAndGeneration = getCacheSnapshot();
+                }
+            }
         }
-        return cache;
+        return cacheAndGeneration.validCache();
     }
 
     /** Caches must only be instantiated using this method */