Revert "Reduce chance of deadlock in account cache"

This reverts commit 00fc15ac00.

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
This commit is contained in:
Hugo Arès 2018-02-27 19:05:41 -05:00
parent 6ed5a40999
commit b4cb044be9
6 changed files with 33 additions and 91 deletions

View File

@ -212,18 +212,6 @@ maven_jar(
sha1 = GUAVA_BIN_SHA1, 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( maven_jar(
name = "velocity", name = "velocity",
artifact = "org.apache.velocity:velocity:1.7", artifact = "org.apache.velocity:velocity:1.7",

View File

@ -8,8 +8,6 @@ java_library(
"//gerrit-common:server", "//gerrit-common:server",
"//gerrit-extension-api:api", "//gerrit-extension-api:api",
"//gerrit-server:server", "//gerrit-server:server",
"//lib:caffeine",
"//lib:caffeine_guava",
"//lib:guava", "//lib:guava",
"//lib:h2", "//lib:h2",
"//lib/guice", "//lib/guice",
@ -24,8 +22,6 @@ junit_tests(
deps = [ deps = [
":cache-h2", ":cache-h2",
"//gerrit-server:server", "//gerrit-server:server",
"//lib:caffeine",
"//lib:caffeine_guava",
"//lib:guava", "//lib:guava",
"//lib:h2", "//lib:h2",
"//lib:junit", "//lib:junit",

View File

@ -14,16 +14,12 @@
package com.google.gerrit.server.cache.h2; 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.base.Strings;
import com.google.common.cache.Cache; import com.google.common.cache.Cache;
import com.google.common.cache.CacheBuilder;
import com.google.common.cache.CacheLoader; import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache; 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.lifecycle.LifecycleModule;
import com.google.gerrit.server.cache.CacheBinding; import com.google.gerrit.server.cache.CacheBinding;
import com.google.gerrit.server.cache.ForwardingRemovalListener; import com.google.gerrit.server.cache.ForwardingRemovalListener;
@ -61,48 +57,35 @@ public class DefaultCacheFactory implements MemoryCacheFactory {
@Override @Override
public <K, V> Cache<K, V> build(CacheBinding<K, V> def) { public <K, V> Cache<K, V> build(CacheBinding<K, V> def) {
return CaffeinatedGuava.build(create(def, false)); return create(def, false).build();
} }
@Override @Override
public <K, V> LoadingCache<K, V> build(CacheBinding<K, V> def, CacheLoader<K, V> loader) { public <K, V> LoadingCache<K, V> build(CacheBinding<K, V> def, CacheLoader<K, V> loader) {
return CaffeinatedGuava.build(create(def, false), loader); return create(def, false).build(loader);
} }
@SuppressWarnings("unchecked") @SuppressWarnings("unchecked")
<K, V> Caffeine<K, V> create(CacheBinding<K, V> def, boolean unwrapValueHolder) { <K, V> CacheBuilder<K, V> create(CacheBinding<K, V> def, boolean unwrapValueHolder) {
Caffeine<K, V> builder = newCacheBuilder(); CacheBuilder<K, V> builder = newCacheBuilder();
builder.recordStats(); builder.recordStats();
builder.maximumWeight(cfg.getLong("cache", def.name(), "memoryLimit", def.maximumWeight())); builder.maximumWeight(cfg.getLong("cache", def.name(), "memoryLimit", def.maximumWeight()));
builder = builder = builder.removalListener(forwardingRemovalListenerFactory.create(def.name()));
builder.removalListener(
new CaffeineToGuavaRemovalListener<K, V>(
forwardingRemovalListenerFactory.create(def.name())));
Weigher<K, V> weigher = null; Weigher<K, V> weigher = def.weigher();
com.google.common.cache.Weigher<K, V> guavaWeigher = def.weigher(); if (weigher != null && unwrapValueHolder) {
if (guavaWeigher != null) { final Weigher<K, V> impl = weigher;
if (unwrapValueHolder) { weigher =
weigher = (Weigher<K, V>)
(Weigher<K, V>) new Weigher<K, ValueHolder<V>>() {
new Weigher<K, ValueHolder<V>>() { @Override
@Override public int weigh(K key, ValueHolder<V> value) {
public int weigh(K key, ValueHolder<V> value) { return impl.weigh(key, value.value);
return guavaWeigher.weigh(key, value.value); }
} };
}; } else if (weigher == null) {
} else { weigher = unitWeight();
weigher =
new Weigher<K, V>() {
@Override
public int weigh(K key, V value) {
return guavaWeigher.weigh(key, value);
}
};
}
} else {
weigher = Weigher.singletonWeigher();
} }
builder.weigher(weigher); builder.weigher(weigher);
@ -124,24 +107,16 @@ public class DefaultCacheFactory implements MemoryCacheFactory {
} }
@SuppressWarnings("unchecked") @SuppressWarnings("unchecked")
private static <K, V> Caffeine<K, V> newCacheBuilder() { private static <K, V> CacheBuilder<K, V> newCacheBuilder() {
return (Caffeine<K, V>) Caffeine.newBuilder(); return (CacheBuilder<K, V>) CacheBuilder.newBuilder();
} }
private static class CaffeineToGuavaRemovalListener<K, V> implements RemovalListener<K, V> { private static <K, V> Weigher<K, V> unitWeight() {
return new Weigher<K, V>() {
private com.google.common.cache.RemovalListener<K, V> guavalistener; @Override
public int weigh(K key, V value) {
private CaffeineToGuavaRemovalListener( return 1;
com.google.common.cache.RemovalListener<K, V> 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())));
}
} }
} }

View File

@ -14,7 +14,6 @@
package com.google.gerrit.server.cache.h2; 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.Cache;
import com.google.common.cache.CacheLoader; import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache; import com.google.common.cache.LoadingCache;
@ -169,7 +168,7 @@ class H2CacheFactory implements PersistentCacheFactory, LifecycleListener {
executor, executor,
store, store,
def.keyType(), def.keyType(),
(Cache<K, ValueHolder<V>>) CaffeinatedGuava.build(defaultFactory.create(def, true))); (Cache<K, ValueHolder<V>>) defaultFactory.create(def, true).build());
synchronized (caches) { synchronized (caches) {
caches.add(cache); caches.add(cache);
} }
@ -189,9 +188,9 @@ class H2CacheFactory implements PersistentCacheFactory, LifecycleListener {
newSqlStore(def.name(), def.keyType(), limit, def.expireAfterWrite(TimeUnit.SECONDS)); newSqlStore(def.name(), def.keyType(), limit, def.expireAfterWrite(TimeUnit.SECONDS));
Cache<K, ValueHolder<V>> mem = Cache<K, ValueHolder<V>> mem =
(Cache<K, ValueHolder<V>>) (Cache<K, ValueHolder<V>>)
CaffeinatedGuava.build( defaultFactory
defaultFactory.create(def, true), .create(def, true)
(CacheLoader<K, V>) new H2CacheImpl.Loader<>(executor, store, loader)); .build((CacheLoader<K, V>) new H2CacheImpl.Loader<>(executor, store, loader));
H2CacheImpl<K, V> cache = new H2CacheImpl<>(executor, store, def.keyType(), mem); H2CacheImpl<K, V> cache = new H2CacheImpl<>(executor, store, def.keyType(), mem);
caches.add(cache); caches.add(cache);
return cache; return cache;

View File

@ -20,8 +20,6 @@ EXPORTS = [
"//gerrit-gwtexpui:server", "//gerrit-gwtexpui:server",
"//gerrit-reviewdb:server", "//gerrit-reviewdb:server",
"//gerrit-server/src/main/prolog:common", "//gerrit-server/src/main/prolog:common",
"//lib:caffeine",
"//lib:caffeine_guava",
"//lib/commons:dbcp", "//lib/commons:dbcp",
"//lib/commons:lang", "//lib/commons:lang",
"//lib/commons:lang3", "//lib/commons:lang3",

View File

@ -82,20 +82,6 @@ java_library(
exports = ["@guava//jar"], 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( java_library(
name = "velocity", name = "velocity",
data = ["//lib:LICENSE-Apache2.0"], data = ["//lib:LICENSE-Apache2.0"],