From ebab79c4cac19b5ee5f0b8bd896e36e4f689547a Mon Sep 17 00:00:00 2001 From: Luck Date: Tue, 1 Nov 2016 19:38:08 +0000 Subject: [PATCH] Maybe fix blocking issue with #getPermissions --- .../common/core/PermissionHolder.java | 160 +++++++++--------- .../lucko/luckperms/common/utils/Cache.java | 52 ++++-- 2 files changed, 125 insertions(+), 87 deletions(-) diff --git a/common/src/main/java/me/lucko/luckperms/common/core/PermissionHolder.java b/common/src/main/java/me/lucko/luckperms/common/core/PermissionHolder.java index 64aa37816..25ca663a3 100644 --- a/common/src/main/java/me/lucko/luckperms/common/core/PermissionHolder.java +++ b/common/src/main/java/me/lucko/luckperms/common/core/PermissionHolder.java @@ -46,7 +46,6 @@ import java.util.*; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReentrantLock; -import java.util.function.Supplier; import java.util.stream.Collectors; /** @@ -72,10 +71,85 @@ public abstract class PermissionHolder { private final Set nodes = new HashSet<>(); private final Set transientNodes = new HashSet<>(); - private Cache> cache = new Cache<>(); - private Cache> mergedCache = new Cache<>(); - private Cache> enduringCache = new Cache<>(); - private Cache> transientCache = new Cache<>(); + private Cache> enduringCache = new Cache<>(() -> { + synchronized (nodes) { + return ImmutableSet.copyOf(nodes); + } + }); + + private Cache> transientCache = new Cache<>(() -> { + synchronized (transientNodes) { + return ImmutableSet.copyOf(transientNodes); + } + }); + + private Cache> cache = new Cache<>(() -> { + TreeSet combined = new TreeSet<>(PriorityComparator.reverse()); + Set enduring = getNodes(); + if (!enduring.isEmpty()) { + combined.addAll(getNodes().stream() + .map(n -> makeLocal(n, getObjectName())) + .collect(Collectors.toList()) + ); + } + Set tran = getTransientNodes(); + if (!tran.isEmpty()) { + combined.addAll(getTransientNodes().stream() + .map(n -> makeLocal(n, getObjectName())) + .collect(Collectors.toList()) + ); + } + + Iterator it = combined.iterator(); + Set higherPriority = new HashSet<>(); + + iterate: + while (it.hasNext()) { + LocalizedNode entry = it.next(); + for (LocalizedNode h : higherPriority) { + if (entry.getNode().almostEquals(h.getNode())) { + it.remove(); + continue iterate; + } + } + higherPriority.add(entry); + } + return ImmutableSortedSet.copyOfSorted(combined); + }); + + private Cache> mergedCache = new Cache<>(() -> { + TreeSet combined = new TreeSet<>(PriorityComparator.reverse()); + Set enduring = getNodes(); + if (!enduring.isEmpty()) { + combined.addAll(getNodes().stream() + .map(n -> makeLocal(n, getObjectName())) + .collect(Collectors.toList()) + ); + } + Set tran = getTransientNodes(); + if (!tran.isEmpty()) { + combined.addAll(getTransientNodes().stream() + .map(n -> makeLocal(n, getObjectName())) + .collect(Collectors.toList()) + ); + } + + Iterator it = combined.iterator(); + Set higherPriority = new HashSet<>(); + + iterate: + while (it.hasNext()) { + LocalizedNode entry = it.next(); + for (LocalizedNode h : higherPriority) { + if (entry.getNode().equalsIgnoringValueOrTemp(h.getNode())) { + it.remove(); + continue iterate; + } + } + higherPriority.add(entry); + } + return ImmutableSortedSet.copyOfSorted(combined); + }); @Getter private final Lock ioLock = new ReentrantLock(); @@ -83,25 +157,11 @@ public abstract class PermissionHolder { public abstract String getFriendlyName(); public Set getNodes() { - Optional> opt = enduringCache.getIfPresent(); - if (opt.isPresent()) { - return opt.get(); - } - - synchronized (nodes) { - return enduringCache.get(() -> ImmutableSet.copyOf(nodes)); - } + return enduringCache.get(); } public Set getTransientNodes() { - Optional> opt = transientCache.getIfPresent(); - if (opt.isPresent()) { - return opt.get(); - } - - synchronized (transientNodes) { - return transientCache.get(() -> ImmutableSet.copyOf(transientNodes)); - } + return transientCache.get(); } private void invalidateCache(boolean enduring) { @@ -119,61 +179,7 @@ public abstract class PermissionHolder { * @return the holders transient and permanent nodes */ public SortedSet getPermissions(boolean mergeTemp) { - Optional> opt = mergeTemp ? mergedCache.getIfPresent() : cache.getIfPresent(); - if (opt.isPresent()) { - return opt.get(); - } - - Supplier> supplier = () -> { - // Create sorted set - TreeSet combined = new TreeSet<>(PriorityComparator.reverse()); - - // Flatten enduring and transient nodes - Set enduring = getNodes(); - if (!enduring.isEmpty()) { - combined.addAll(getNodes().stream() - .map(n -> makeLocal(n, getObjectName())) - .collect(Collectors.toList()) - ); - } - - Set tran = getTransientNodes(); - if (!tran.isEmpty()) { - combined.addAll(getTransientNodes().stream() - .map(n -> makeLocal(n, getObjectName())) - .collect(Collectors.toList()) - ); - } - - // Create an iterator over all permissions being considered - Iterator it = combined.iterator(); - - // Temporary set to store high priority values - Set higherPriority = new HashSet<>(); - - // Iterate through each node being considered - iterate: - while (it.hasNext()) { - LocalizedNode entry = it.next(); - - // Check through all of the higher priority nodes - for (LocalizedNode h : higherPriority) { - - // Check to see if the entry being considered was already processed at a higher priority - if (mergeTemp ? entry.getNode().equalsIgnoringValueOrTemp(h.getNode()) : entry.getNode().almostEquals(h.getNode())) { - it.remove(); - continue iterate; - } - } - - // This entry will be kept. - higherPriority.add(entry); - } - - return ImmutableSortedSet.copyOfSorted(combined); - }; - - return mergeTemp ? mergedCache.get(supplier) : cache.get(supplier); + return mergeTemp ? mergedCache.get() : cache.get(); } /** @@ -239,7 +245,7 @@ public abstract class PermissionHolder { excludedGroups.add(getObjectName().toLowerCase()); - Set parents = getPermissions(true).stream() + Set parents = all.stream() .map(LocalizedNode::getNode) .filter(Node::getValue) .filter(Node::isGroupNode) diff --git a/common/src/main/java/me/lucko/luckperms/common/utils/Cache.java b/common/src/main/java/me/lucko/luckperms/common/utils/Cache.java index a166ff125..ce208499b 100644 --- a/common/src/main/java/me/lucko/luckperms/common/utils/Cache.java +++ b/common/src/main/java/me/lucko/luckperms/common/utils/Cache.java @@ -22,30 +22,62 @@ package me.lucko.luckperms.common.utils; +import lombok.RequiredArgsConstructor; + import java.util.Optional; +import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.function.Supplier; +/** + * Thread-safe caching utility + * @param the type being stored + */ +@RequiredArgsConstructor public class Cache { - private T t = null; + private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock(); + private final Supplier supplier; - public T get(Supplier supplier) { - synchronized (this) { - if (t == null) { - t = supplier.get(); + private T cached = null; + + public T get() { + lock.readLock().lock(); + try { + if (cached != null) { + return cached; } - return t; + } finally { + lock.readLock().unlock(); + } + + lock.writeLock().lock(); + try { + // Check again + if (cached != null) { + return cached; + } + + cached = supplier.get(); + return cached; + } finally { + lock.writeLock().unlock(); } } public Optional getIfPresent() { - synchronized (this) { - return Optional.ofNullable(t); + lock.readLock().lock(); + try { + return Optional.ofNullable(cached); + } finally { + lock.readLock().unlock(); } } public void invalidate() { - synchronized (this) { - t = null; + lock.writeLock().lock(); + try { + cached = null; + } finally { + lock.writeLock().unlock(); } } }