Use ConcurrentHashMap instead of Caffeine in CachedDataManager

The behaviour of Caffeine cache invalidation calls is "undefined for an entry that is being loaded (or reloaded)" - this causes a nasty race condition in CachedDataManager, because we rely on the call to #invalidate to completely clear the cache and prevent old/outdated data from sticking around & being used for permission queries.

This is an unfortunate characteristic of Caffeine, because other than that, it is perfect for our use-case.
This commit is contained in:
Luck 2020-07-03 17:57:53 +01:00
parent 8b506b6a95
commit adbd2fc81f
No known key found for this signature in database
GPG Key ID: EFA9B3EC5FD90F8B
4 changed files with 73 additions and 33 deletions

View File

@ -25,9 +25,7 @@
package me.lucko.luckperms.common.cacheddata;
import com.github.benmanes.caffeine.cache.CacheLoader;
import com.github.benmanes.caffeine.cache.LoadingCache;
import me.lucko.luckperms.common.cache.LoadingMap;
import me.lucko.luckperms.common.cache.MRUCache;
import me.lucko.luckperms.common.cacheddata.type.MetaAccumulator;
import me.lucko.luckperms.common.cacheddata.type.MetaCache;
@ -53,6 +51,7 @@ import java.util.Set;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.TimeUnit;
import java.util.function.Function;
/**
* Abstract implementation of {@link CachedDataManager}.
@ -176,23 +175,25 @@ public abstract class AbstractCachedDataManager implements CachedDataManager {
@Override
public final void invalidatePermissionCalculators() {
this.permission.cache.asMap().values().forEach(PermissionCache::invalidateCache);
this.permission.cache.values().forEach(PermissionCache::invalidateCache);
}
public final void performCacheCleanup() {
this.permission.cache.cleanUp();
this.meta.cache.cleanUp();
this.permission.cleanup();
this.meta.cleanup();
}
private static final class AbstractContainer<C extends I, I extends CachedData> extends MRUCache<RecentData<C>> implements Container<I> {
private final Loader<QueryOptions, C> cacheLoader;
private final LoadingCache<QueryOptions, C> cache;
private final Function<QueryOptions, C> cacheLoader;
private final LoadingMap<QueryOptions, C> cache;
public AbstractContainer(Loader<QueryOptions, C> cacheLoader) {
public AbstractContainer(Function<QueryOptions, C> cacheLoader) {
this.cacheLoader = cacheLoader;
this.cache = CaffeineFactory.newBuilder()
.expireAfterAccess(2, TimeUnit.MINUTES)
.build(this.cacheLoader);
this.cache = LoadingMap.of(this.cacheLoader);
}
public void cleanup() {
this.cache.values().removeIf(value -> ((UsageTracked) value).usedSince(TimeUnit.MINUTES.toMillis(2)));
}
@Override
@ -201,28 +202,31 @@ public abstract class AbstractCachedDataManager implements CachedDataManager {
RecentData<C> recent = getRecent();
if (recent != null && queryOptions.equals(recent.queryOptions)) {
((UsageTracked) recent.data).recordUsage();
return recent.data;
}
int modCount = modCount();
C data = this.cache.get(queryOptions);
((UsageTracked) data).recordUsage();
offerRecent(modCount, new RecentData<>(queryOptions, data));
//noinspection ConstantConditions
return data;
}
@Override
public @NonNull C calculate(@NonNull QueryOptions queryOptions) {
Objects.requireNonNull(queryOptions, "queryOptions");
return this.cacheLoader.load(queryOptions);
return this.cacheLoader.apply(queryOptions);
}
@Override
public void recalculate(@NonNull QueryOptions queryOptions) {
Objects.requireNonNull(queryOptions, "queryOptions");
this.cache.refresh(queryOptions);
clearRecent();
CompletableFuture.runAsync(() -> {
final C value = this.cacheLoader.apply(queryOptions);
this.cache.put(queryOptions, value);
clearRecent();
}, CaffeineFactory.executor());
}
@Override
@ -230,47 +234,43 @@ public abstract class AbstractCachedDataManager implements CachedDataManager {
Objects.requireNonNull(queryOptions, "queryOptions");
// invalidate the previous value until we're done recalculating
this.cache.invalidate(queryOptions);
this.cache.remove(queryOptions);
clearRecent();
// request recalculation from the cache
return CompletableFuture.supplyAsync(
() -> this.cache.get(queryOptions),
CaffeineFactory.executor()
);
return CompletableFuture.supplyAsync(() -> {
C value = this.cache.get(queryOptions);
clearRecent();
return value;
}, CaffeineFactory.executor());
}
@Override
public void recalculate() {
Set<QueryOptions> keys = this.cache.asMap().keySet();
Set<QueryOptions> keys = this.cache.keySet();
keys.forEach(this::recalculate);
}
@Override
public @NonNull CompletableFuture<Void> reload() {
Set<QueryOptions> keys = this.cache.asMap().keySet();
Set<QueryOptions> keys = this.cache.keySet();
return CompletableFuture.allOf(keys.stream().map(this::reload).toArray(CompletableFuture[]::new));
}
@Override
public void invalidate(@NonNull QueryOptions queryOptions) {
Objects.requireNonNull(queryOptions, "queryOptions");
this.cache.invalidate(queryOptions);
this.cache.remove(queryOptions);
clearRecent();
}
@Override
public void invalidate() {
this.cache.invalidateAll();
this.cache.clear();
clearRecent();
}
}
private interface Loader<K, V> extends CacheLoader<K, V> {
@Override
@NonNull V load(@NonNull K key);
}
private MetaStackDefinition getMetaStackDefinition(QueryOptions queryOptions, ChatMetaType type) {
MetaStackDefinition stack = queryOptions.option(type == ChatMetaType.PREFIX ?
MetaStackDefinition.PREFIX_STACK_KEY :

View File

@ -0,0 +1,38 @@
/*
* This file is part of LuckPerms, licensed under the MIT License.
*
* Copyright (c) lucko (Luck) <luck@lucko.me>
* Copyright (c) contributors
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in all
* copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
* SOFTWARE.
*/
package me.lucko.luckperms.common.cacheddata;
public abstract class UsageTracked {
private long lastUsed = System.currentTimeMillis();
public void recordUsage() {
this.lastUsed = System.currentTimeMillis();
}
public boolean usedSince(long duration) {
return this.lastUsed > (System.currentTimeMillis() - duration);
}
}

View File

@ -26,6 +26,7 @@
package me.lucko.luckperms.common.cacheddata.type;
import me.lucko.luckperms.common.cacheddata.CacheMetadata;
import me.lucko.luckperms.common.cacheddata.UsageTracked;
import me.lucko.luckperms.common.calculator.CalculatorFactory;
import me.lucko.luckperms.common.calculator.PermissionCalculator;
import me.lucko.luckperms.common.calculator.result.TristateResult;
@ -44,7 +45,7 @@ import java.util.concurrent.ConcurrentHashMap;
/**
* Holds cached permissions data for a given context
*/
public class PermissionCache implements CachedPermissionData {
public class PermissionCache extends UsageTracked implements CachedPermissionData {
/**
* The query options this container is holding data for

View File

@ -30,6 +30,7 @@ import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSortedMap;
import com.google.common.collect.Multimaps;
import me.lucko.luckperms.common.cacheddata.UsageTracked;
import me.lucko.luckperms.common.config.ConfigKeys;
import me.lucko.luckperms.common.plugin.LuckPermsPlugin;
import me.lucko.luckperms.common.verbose.event.MetaCheckEvent;
@ -50,7 +51,7 @@ import java.util.SortedMap;
/**
* Holds cached meta for a given context
*/
public class SimpleMetaCache implements CachedMetaData {
public class SimpleMetaCache extends UsageTracked implements CachedMetaData {
private final LuckPermsPlugin plugin;