From 5065623ab0b184401405040ed3608df300f322c6 Mon Sep 17 00:00:00 2001 From: Ivan Pekov Date: Thu, 6 Aug 2020 20:54:35 +0300 Subject: [PATCH] Attempt at fixing 413 (#422) * Attempt at fixing 413 This is my (miserable) attempt at fixing #413 These changes basically fix some potential threading issues and (probably) #413 Local tests went fine for me, but more tests are required. * Remove delay, fixed -> cached thread pool --- .../manager/CloudExpansionManager.java | 115 +++++++++++------- .../manager/LocalExpansionManager.java | 70 ++++++++--- 2 files changed, 118 insertions(+), 67 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 6e77bbb..36603e9 100644 --- a/src/main/java/me/clip/placeholderapi/expansion/manager/CloudExpansionManager.java +++ b/src/main/java/me/clip/placeholderapi/expansion/manager/CloudExpansionManager.java @@ -22,6 +22,7 @@ package me.clip.placeholderapi.expansion.manager; import com.google.common.collect.ImmutableMap; import com.google.common.io.Resources; +import com.google.common.util.concurrent.ThreadFactoryBuilder; import com.google.gson.Gson; import com.google.gson.reflect.TypeToken; import java.io.File; @@ -32,14 +33,18 @@ import java.net.URL; import java.nio.channels.Channels; import java.nio.channels.ReadableByteChannel; import java.nio.charset.StandardCharsets; +import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import java.util.function.Function; import java.util.logging.Level; import java.util.stream.Collector; @@ -73,6 +78,9 @@ public final class CloudExpansionManager { @NotNull private final Map> await = new ConcurrentHashMap<>(); + private final ExecutorService ASYNC_EXECUTOR = + Executors.newCachedThreadPool( + new ThreadFactoryBuilder().setNameFormat("placeholderapi-io-#%1$d").build()); public CloudExpansionManager(@NotNull final PlaceholderAPIPlugin plugin) { this.plugin = plugin; @@ -163,58 +171,71 @@ public final class CloudExpansionManager { public void fetch(final boolean allowUnverified) { plugin.getLogger().info("Fetching available expansion information..."); - CompletableFuture> future = CompletableFuture.supplyAsync(() -> { - final Map values = new HashMap<>(); + ASYNC_EXECUTOR.submit( + () -> { + // a defence tactic! use ConcurrentHashMap instead of normal HashMap + Map values = new ConcurrentHashMap<>(); + try { + //noinspection UnstableApiUsage + String json = Resources.toString(new URL(API_URL), StandardCharsets.UTF_8); + values.putAll(GSON.fromJson(json, TYPE)); - try { - //noinspection UnstableApiUsage - final String json = Resources.toString(new URL(API_URL), StandardCharsets.UTF_8); - values.putAll(GSON.fromJson(json, TYPE)); - } catch (final IOException ex) { - throw new CompletionException(ex); - } + List toRemove = new ArrayList<>(); - values.values().removeIf(value -> value.getLatestVersion() == null - || value.getVersion(value.getLatestVersion()) == null); + for (Map.Entry entry : values.entrySet()) { + CloudExpansion expansion = entry.getValue(); + if (expansion.getLatestVersion() == null + || expansion.getVersion(expansion.getLatestVersion()) == null) { + toRemove.add(entry.getKey()); + } + if (!allowUnverified && !expansion.isVerified()) { + toRemove.add(entry.getKey()); + } + } - return values; - }); + for (String name : toRemove) { + values.remove(name); + } + } catch (Throwable e) { + // ugly swallowing of every throwable, but we have to be defensive + plugin.getLogger().log(Level.WARNING, "Failed to download expansion information", e); + } - if (!allowUnverified) { - future = future.thenApplyAsync((values) -> { - values.values().removeIf(expansion -> !expansion.isVerified()); - return values; - }); - } + // loop thru what's left on the main thread + plugin + .getServer() + .getScheduler() + .runTask( + plugin, + () -> { + try { + for (Map.Entry entry : values.entrySet()) { + String name = entry.getKey(); + CloudExpansion expansion = entry.getValue(); - future = future.thenApplyAsync((values) -> { + expansion.setName(name); - values.forEach((name, expansion) -> { - expansion.setName(name); + Optional localOpt = + plugin.getLocalExpansionManager().findExpansionByName(name); + if (localOpt.isPresent()) { + PlaceholderExpansion local = localOpt.get(); + if (local.isRegistered()) { + expansion.setHasExpansion(true); + expansion.setShouldUpdate( + !local.getVersion().equalsIgnoreCase(expansion.getLatestVersion())); + } + } - final Optional local = plugin.getLocalExpansionManager() - .findExpansionByName(name); - if (local.isPresent() && local.get().isRegistered()) { - expansion.setHasExpansion(true); - expansion.setShouldUpdate(!local.get().getVersion().equals(expansion.getLatestVersion())); - } - }); - - return values; - }); - - future.whenComplete((expansions, exception) -> { - - if (exception != null) { - plugin.getLogger() - .log(Level.WARNING, "failed to download expansion information", exception); - return; - } - - for (final CloudExpansion expansion : expansions.values()) { - this.cache.put(toIndexName(expansion), expansion); - } - }); + cache.put(toIndexName(expansion), expansion); + } + } catch (Throwable e) { + // ugly swallowing of every throwable, but we have to be defensive + plugin + .getLogger() + .log(Level.WARNING, "Failed to download expansion information", e); + } + }); + }); } public boolean isDownloading(@NotNull final CloudExpansion expansion) { @@ -240,7 +261,7 @@ public final class CloudExpansionManager { throw new CompletionException(ex); } return file; - }); + }, ASYNC_EXECUTOR); download.whenCompleteAsync((value, exception) -> { await.remove(toIndexName(expansion)); @@ -249,7 +270,7 @@ public final class CloudExpansionManager { plugin.getLogger().log(Level.SEVERE, "failed to download " + expansion.getName() + ":" + version.getVersion(), exception); } - }); + }, ASYNC_EXECUTOR); await.put(toIndexName(expansion), download); diff --git a/src/main/java/me/clip/placeholderapi/expansion/manager/LocalExpansionManager.java b/src/main/java/me/clip/placeholderapi/expansion/manager/LocalExpansionManager.java index 1867704..79f3509 100644 --- a/src/main/java/me/clip/placeholderapi/expansion/manager/LocalExpansionManager.java +++ b/src/main/java/me/clip/placeholderapi/expansion/manager/LocalExpansionManager.java @@ -20,18 +20,18 @@ package me.clip.placeholderapi.expansion.manager; -import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; import java.io.File; import java.util.Arrays; import java.util.Collection; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.locks.ReentrantLock; import java.util.logging.Level; import me.clip.placeholderapi.PlaceholderAPIPlugin; import me.clip.placeholderapi.events.ExpansionRegisterEvent; @@ -72,7 +72,8 @@ public final class LocalExpansionManager implements Listener { private final PlaceholderAPIPlugin plugin; @NotNull - private final Map expansions = new HashMap<>(); + private final Map expansions = new ConcurrentHashMap<>(); + private final ReentrantLock expansionsLock = new ReentrantLock(); public LocalExpansionManager(@NotNull final PlaceholderAPIPlugin plugin) { @@ -98,31 +99,54 @@ public final class LocalExpansionManager implements Listener { return folder; } - public int getExpansionsCount() { - return expansions.size(); - } - - @NotNull @Unmodifiable public Collection getIdentifiers() { - return ImmutableSet.copyOf(expansions.keySet()); + expansionsLock.lock(); + try { + return ImmutableSet.copyOf(expansions.keySet()); + } finally { + expansionsLock.unlock(); + } } @NotNull @Unmodifiable public Collection getExpansions() { - return ImmutableSet.copyOf(expansions.values()); + expansionsLock.lock(); + try { + return ImmutableSet.copyOf(expansions.values()); + } finally { + expansionsLock.unlock(); + } } @Nullable public PlaceholderExpansion getExpansion(@NotNull final String identifier) { - return ImmutableMap.copyOf(expansions).get(identifier.toLowerCase()); + expansionsLock.lock(); + try { + return expansions.get(identifier.toLowerCase()); + } finally { + expansionsLock.unlock(); + } } @NotNull public Optional findExpansionByName(@NotNull final String name) { - return getExpansions().stream().filter(expansion -> name.equalsIgnoreCase(expansion.getName())).findFirst(); + expansionsLock.lock(); + try { + PlaceholderExpansion bestMatch = null; + for (Map.Entry entry : expansions.entrySet()) { + PlaceholderExpansion expansion = entry.getValue(); + if (expansion.getName().equalsIgnoreCase(name)) { + bestMatch = expansion; + break; + } + } + return Optional.ofNullable(bestMatch); + } finally { + expansionsLock.unlock(); + } } @NotNull @@ -203,7 +227,7 @@ public final class LocalExpansionManager implements Listener { } } - final PlaceholderExpansion removed = expansions.get(identifier); + final PlaceholderExpansion removed = getExpansion(identifier); if (removed != null && !removed.unregister()) { return false; } @@ -215,7 +239,12 @@ public final class LocalExpansionManager implements Listener { return false; } - expansions.put(identifier, expansion); + expansionsLock.lock(); + try { + expansions.put(identifier, expansion); + } finally { + expansionsLock.unlock(); + } if (expansion instanceof Listener) { Bukkit.getPluginManager().registerEvents(((Listener) expansion), plugin); @@ -228,12 +257,13 @@ public final class LocalExpansionManager implements Listener { } if (plugin.getPlaceholderAPIConfig().isCloudEnabled()) { - final Optional cloudExpansion = plugin.getCloudExpansionManager() - .findCloudExpansionByName(identifier); - if (cloudExpansion.isPresent()) { - cloudExpansion.get().setHasExpansion(true); - cloudExpansion.get().setShouldUpdate( - !cloudExpansion.get().getLatestVersion().equals(expansion.getVersion())); + final Optional cloudExpansionOptional = + plugin.getCloudExpansionManager().findCloudExpansionByName(identifier); + if (cloudExpansionOptional.isPresent()) { + CloudExpansion cloudExpansion = cloudExpansionOptional.get(); + cloudExpansion.setHasExpansion(true); + cloudExpansion.setShouldUpdate( + !cloudExpansion.getLatestVersion().equals(expansion.getVersion())); } }