From ab11e36206fb29785c5a91c921c6258805fd31b5 Mon Sep 17 00:00:00 2001 From: Ivan Pekov Date: Wed, 3 Nov 2021 16:25:48 +0200 Subject: [PATCH] cacheLock and awaitLock --- .../manager/CloudExpansionManager.java | 114 ++++++++++++++---- .../me/clip/placeholderapi/util/Futures.java | 1 - 2 files changed, 89 insertions(+), 26 deletions(-) diff --git a/src/main/java/me/clip/placeholderapi/expansion/manager/CloudExpansionManager.java b/src/main/java/me/clip/placeholderapi/expansion/manager/CloudExpansionManager.java index 4d6cd6b..b48ba3d 100644 --- a/src/main/java/me/clip/placeholderapi/expansion/manager/CloudExpansionManager.java +++ b/src/main/java/me/clip/placeholderapi/expansion/manager/CloudExpansionManager.java @@ -46,6 +46,7 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.ReentrantLock; import java.util.function.Function; import java.util.logging.Level; import java.util.stream.Collector; @@ -76,8 +77,10 @@ public final class CloudExpansionManager { @NotNull private final Map cache = new HashMap<>(); + private final ReentrantLock cacheLock = new ReentrantLock(); @NotNull private final Map> await = new ConcurrentHashMap<>(); + private final ReentrantLock awaitLock = new ReentrantLock(); static final ExecutorService ASYNC_EXECUTOR = Executors.newCachedThreadPool( @@ -113,39 +116,59 @@ public final class CloudExpansionManager { @NotNull @Unmodifiable public Map getCloudExpansions() { - return ImmutableMap.copyOf(cache); + cacheLock.lock(); + try { + return ImmutableMap.copyOf(cache); + } finally { + cacheLock.unlock(); + } } @NotNull @Unmodifiable public Map getCloudExpansionsInstalled() { - if (cache.isEmpty()) { - return Collections.emptyMap(); - } + cacheLock.lock(); + try { + if (cache.isEmpty()) { + return Collections.emptyMap(); + } - return cache.values() - .stream() - .filter(CloudExpansion::hasExpansion) - .collect(INDEXED_NAME_COLLECTOR); + return cache.values() + .stream() + .filter(CloudExpansion::hasExpansion) + .collect(INDEXED_NAME_COLLECTOR); + } finally { + cacheLock.unlock(); + } } @NotNull @Unmodifiable public Map getCloudExpansionsByAuthor(@NotNull final String author) { - if (cache.isEmpty()) { - return Collections.emptyMap(); - } + cacheLock.lock(); + try { + if (cache.isEmpty()) { + return Collections.emptyMap(); + } - return cache.values() - .stream() - .filter(expansion -> author.equalsIgnoreCase(expansion.getAuthor())) - .collect(INDEXED_NAME_COLLECTOR); + return cache.values() + .stream() + .filter(expansion -> author.equalsIgnoreCase(expansion.getAuthor())) + .collect(INDEXED_NAME_COLLECTOR); + } finally { + cacheLock.unlock(); + } } @NotNull @Unmodifiable public Set getCloudExpansionAuthors() { - return cache.values().stream().map(CloudExpansion::getAuthor).collect(Collectors.toSet()); + cacheLock.lock(); + try { + return cache.values().stream().map(CloudExpansion::getAuthor).collect(Collectors.toSet()); + } finally { + cacheLock.unlock(); + } } public int getCloudExpansionAuthorCount() { @@ -163,14 +186,29 @@ public final class CloudExpansionManager { @NotNull public Optional findCloudExpansionByName(@NotNull final String name) { - return Optional.ofNullable(cache.get(toIndexName(name))); + cacheLock.lock(); + try { + return Optional.ofNullable(cache.get(toIndexName(name))); + } finally { + cacheLock.unlock(); + } } public void clean() { - cache.clear(); + cacheLock.lock(); + try { + cache.clear(); + } finally { + cacheLock.unlock(); + } - await.values().forEach(future -> future.cancel(true)); - await.clear(); + awaitLock.lock(); + try { + await.values().forEach(future -> future.cancel(true)); + await.clear(); + } finally { + awaitLock.unlock(); + } } public void fetch(final boolean allowUnverified) { @@ -231,7 +269,12 @@ public final class CloudExpansionManager { } } - cache.put(toIndexName(expansion), expansion); + cacheLock.lock(); + try { + cache.put(toIndexName(expansion), expansion); + } finally { + cacheLock.unlock(); + } } } catch (Throwable e) { // ugly swallowing of every throwable, but we have to be defensive @@ -244,13 +287,24 @@ public final class CloudExpansionManager { } public boolean isDownloading(@NotNull final CloudExpansion expansion) { - return await.containsKey(toIndexName(expansion)); + awaitLock.lock(); + try { + return await.containsKey(toIndexName(expansion)); + } finally { + awaitLock.unlock(); + } } @NotNull public CompletableFuture downloadExpansion(@NotNull final CloudExpansion expansion, @NotNull final CloudExpansion.Version version) { - final CompletableFuture previous = await.get(toIndexName(expansion)); + CompletableFuture previous; + awaitLock.lock(); + try { + previous = await.get(toIndexName(expansion)); + } finally { + awaitLock.unlock(); + } if (previous != null) { return previous; } @@ -269,7 +323,12 @@ public final class CloudExpansionManager { }, ASYNC_EXECUTOR); download.whenCompleteAsync((value, exception) -> { - await.remove(toIndexName(expansion)); + awaitLock.lock(); + try { + await.remove(toIndexName(expansion)); + } finally { + awaitLock.unlock(); + } if (exception != null) { plugin.getLogger().log(Level.SEVERE, @@ -277,7 +336,12 @@ public final class CloudExpansionManager { } }, ASYNC_EXECUTOR); - await.put(toIndexName(expansion), download); + awaitLock.lock(); + try { + await.put(toIndexName(expansion), download); + } finally { + awaitLock.unlock(); + } return download; } diff --git a/src/main/java/me/clip/placeholderapi/util/Futures.java b/src/main/java/me/clip/placeholderapi/util/Futures.java index ee60de5..8f07e32 100644 --- a/src/main/java/me/clip/placeholderapi/util/Futures.java +++ b/src/main/java/me/clip/placeholderapi/util/Futures.java @@ -71,7 +71,6 @@ public final class Futures { @NotNull private static List awaitCompletion( @NotNull final Collection> futures) { - // TODO: Calling CompletableFuture#join is bad. Find a way to optimise this whole class return futures.stream().map(CompletableFuture::join).collect(Collectors.toList()); }