From b4cb044be939a7653b031959da88053da2646931 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Tue, 27 Feb 2018 19:05:41 -0500 Subject: [PATCH] Revert "Reduce chance of deadlock in account cache" This reverts commit 00fc15ac0073b86270e7c0f40d386f95dfe31e86. With Caffeine, accessing the cache from the cache loader is causing an infinite loop [1]. Merging that change up to stable-2.15 exposed that there is still at least one cache loader (PatchListLoader) accessing its cache (PatchListCacheImpl). The test going into an infinite loop on stable-2.15 is RevisionDiffIT.rebaseHunksOneLineApartFromRegularHunkAreIdentified. Revert the change until circular dependency is removed or a solution is found to access the cache from the cache loader. [1]https://github.com/ben-manes/caffeine/issues/89 Bug: Issue 8464 Change-Id: If65560b4a9bfcf0a03decaedd83ad000c6b28f4f --- WORKSPACE | 12 --- gerrit-cache-h2/BUILD | 4 - .../server/cache/h2/DefaultCacheFactory.java | 83 +++++++------------ .../server/cache/h2/H2CacheFactory.java | 9 +- gerrit-plugin-api/BUILD | 2 - lib/BUILD | 14 ---- 6 files changed, 33 insertions(+), 91 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index d7ce2f38b8..7641e8019b 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -212,18 +212,6 @@ maven_jar( sha1 = GUAVA_BIN_SHA1, ) -maven_jar( - name = "caffeine", - artifact = "com.github.ben-manes.caffeine:caffeine:2.6.1", - sha1 = "fc7a29feda0a3c5aaf1ff55e0df5417025e6d5f4", -) - -maven_jar( - name = "caffeine_guava", - artifact = "com.github.ben-manes.caffeine:guava:2.6.1", - sha1 = "e1fbe0d8c06639d6fee74404f687f00da25671eb", -) - maven_jar( name = "velocity", artifact = "org.apache.velocity:velocity:1.7", diff --git a/gerrit-cache-h2/BUILD b/gerrit-cache-h2/BUILD index 55d47c2c41..45cf4167a4 100644 --- a/gerrit-cache-h2/BUILD +++ b/gerrit-cache-h2/BUILD @@ -8,8 +8,6 @@ java_library( "//gerrit-common:server", "//gerrit-extension-api:api", "//gerrit-server:server", - "//lib:caffeine", - "//lib:caffeine_guava", "//lib:guava", "//lib:h2", "//lib/guice", @@ -24,8 +22,6 @@ junit_tests( deps = [ ":cache-h2", "//gerrit-server:server", - "//lib:caffeine", - "//lib:caffeine_guava", "//lib:guava", "//lib:h2", "//lib:junit", diff --git a/gerrit-cache-h2/src/main/java/com/google/gerrit/server/cache/h2/DefaultCacheFactory.java b/gerrit-cache-h2/src/main/java/com/google/gerrit/server/cache/h2/DefaultCacheFactory.java index 7b85834f50..356695592d 100644 --- a/gerrit-cache-h2/src/main/java/com/google/gerrit/server/cache/h2/DefaultCacheFactory.java +++ b/gerrit-cache-h2/src/main/java/com/google/gerrit/server/cache/h2/DefaultCacheFactory.java @@ -14,16 +14,12 @@ package com.google.gerrit.server.cache.h2; -import com.github.benmanes.caffeine.cache.Caffeine; -import com.github.benmanes.caffeine.cache.RemovalCause; -import com.github.benmanes.caffeine.cache.RemovalListener; -import com.github.benmanes.caffeine.cache.Weigher; -import com.github.benmanes.caffeine.guava.CaffeinatedGuava; import com.google.common.base.Strings; import com.google.common.cache.Cache; +import com.google.common.cache.CacheBuilder; import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; -import com.google.common.cache.RemovalNotification; +import com.google.common.cache.Weigher; import com.google.gerrit.lifecycle.LifecycleModule; import com.google.gerrit.server.cache.CacheBinding; import com.google.gerrit.server.cache.ForwardingRemovalListener; @@ -61,48 +57,35 @@ public class DefaultCacheFactory implements MemoryCacheFactory { @Override public Cache build(CacheBinding def) { - return CaffeinatedGuava.build(create(def, false)); + return create(def, false).build(); } @Override public LoadingCache build(CacheBinding def, CacheLoader loader) { - return CaffeinatedGuava.build(create(def, false), loader); + return create(def, false).build(loader); } @SuppressWarnings("unchecked") - Caffeine create(CacheBinding def, boolean unwrapValueHolder) { - Caffeine builder = newCacheBuilder(); + CacheBuilder create(CacheBinding def, boolean unwrapValueHolder) { + CacheBuilder builder = newCacheBuilder(); builder.recordStats(); builder.maximumWeight(cfg.getLong("cache", def.name(), "memoryLimit", def.maximumWeight())); - builder = - builder.removalListener( - new CaffeineToGuavaRemovalListener( - forwardingRemovalListenerFactory.create(def.name()))); + builder = builder.removalListener(forwardingRemovalListenerFactory.create(def.name())); - Weigher weigher = null; - com.google.common.cache.Weigher guavaWeigher = def.weigher(); - if (guavaWeigher != null) { - if (unwrapValueHolder) { - weigher = - (Weigher) - new Weigher>() { - @Override - public int weigh(K key, ValueHolder value) { - return guavaWeigher.weigh(key, value.value); - } - }; - } else { - weigher = - new Weigher() { - @Override - public int weigh(K key, V value) { - return guavaWeigher.weigh(key, value); - } - }; - } - } else { - weigher = Weigher.singletonWeigher(); + Weigher weigher = def.weigher(); + if (weigher != null && unwrapValueHolder) { + final Weigher impl = weigher; + weigher = + (Weigher) + new Weigher>() { + @Override + public int weigh(K key, ValueHolder value) { + return impl.weigh(key, value.value); + } + }; + } else if (weigher == null) { + weigher = unitWeight(); } builder.weigher(weigher); @@ -124,24 +107,16 @@ public class DefaultCacheFactory implements MemoryCacheFactory { } @SuppressWarnings("unchecked") - private static Caffeine newCacheBuilder() { - return (Caffeine) Caffeine.newBuilder(); + private static CacheBuilder newCacheBuilder() { + return (CacheBuilder) CacheBuilder.newBuilder(); } - private static class CaffeineToGuavaRemovalListener implements RemovalListener { - - private com.google.common.cache.RemovalListener guavalistener; - - private CaffeineToGuavaRemovalListener( - com.google.common.cache.RemovalListener guavalistener) { - this.guavalistener = guavalistener; - } - - @Override - public void onRemoval(K key, V value, RemovalCause cause) { - guavalistener.onRemoval( - RemovalNotification.create( - key, value, com.google.common.cache.RemovalCause.valueOf(cause.name()))); - } + private static Weigher unitWeight() { + return new Weigher() { + @Override + public int weigh(K key, V value) { + return 1; + } + }; } } diff --git a/gerrit-cache-h2/src/main/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java b/gerrit-cache-h2/src/main/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java index 1720eb012a..474fa366d8 100644 --- a/gerrit-cache-h2/src/main/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java +++ b/gerrit-cache-h2/src/main/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java @@ -14,7 +14,6 @@ package com.google.gerrit.server.cache.h2; -import com.github.benmanes.caffeine.guava.CaffeinatedGuava; import com.google.common.cache.Cache; import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; @@ -169,7 +168,7 @@ class H2CacheFactory implements PersistentCacheFactory, LifecycleListener { executor, store, def.keyType(), - (Cache>) CaffeinatedGuava.build(defaultFactory.create(def, true))); + (Cache>) defaultFactory.create(def, true).build()); synchronized (caches) { caches.add(cache); } @@ -189,9 +188,9 @@ class H2CacheFactory implements PersistentCacheFactory, LifecycleListener { newSqlStore(def.name(), def.keyType(), limit, def.expireAfterWrite(TimeUnit.SECONDS)); Cache> mem = (Cache>) - CaffeinatedGuava.build( - defaultFactory.create(def, true), - (CacheLoader) new H2CacheImpl.Loader<>(executor, store, loader)); + defaultFactory + .create(def, true) + .build((CacheLoader) new H2CacheImpl.Loader<>(executor, store, loader)); H2CacheImpl cache = new H2CacheImpl<>(executor, store, def.keyType(), mem); caches.add(cache); return cache; diff --git a/gerrit-plugin-api/BUILD b/gerrit-plugin-api/BUILD index 5f50e2e57f..067b4cd38c 100644 --- a/gerrit-plugin-api/BUILD +++ b/gerrit-plugin-api/BUILD @@ -20,8 +20,6 @@ EXPORTS = [ "//gerrit-gwtexpui:server", "//gerrit-reviewdb:server", "//gerrit-server/src/main/prolog:common", - "//lib:caffeine", - "//lib:caffeine_guava", "//lib/commons:dbcp", "//lib/commons:lang", "//lib/commons:lang3", diff --git a/lib/BUILD b/lib/BUILD index 8e752c0a4a..91334cb83d 100644 --- a/lib/BUILD +++ b/lib/BUILD @@ -82,20 +82,6 @@ java_library( exports = ["@guava//jar"], ) -java_library( - name = "caffeine", - data = ["//lib:LICENSE-Apache2.0"], - visibility = ["//visibility:public"], - exports = ["@caffeine//jar"], -) - -java_library( - name = "caffeine_guava", - data = ["//lib:LICENSE-Apache2.0"], - visibility = ["//visibility:public"], - exports = ["@caffeine_guava//jar"], -) - java_library( name = "velocity", data = ["//lib:LICENSE-Apache2.0"],