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

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<String, CompletableFuture<File>> 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<Map<String, CloudExpansion>> future = CompletableFuture.supplyAsync(() -> {
+    ASYNC_EXECUTOR.submit(
-      final Map<String, CloudExpansion> values = new HashMap<>();
+        () -> {
+          // a defence tactic! use ConcurrentHashMap instead of normal HashMap
+          Map<String, CloudExpansion> values = new ConcurrentHashMap<>();
+          try {
+            //noinspection UnstableApiUsage
+            String json = Resources.toString(new URL(API_URL), StandardCharsets.UTF_8);
+            values.putAll(GSON.fromJson(json, TYPE));
 
-      try {
+            List<String> toRemove = new ArrayList<>();
-        //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);
-      }
 
-      values.values().removeIf(value -> value.getLatestVersion() == null
+            for (Map.Entry<String, CloudExpansion> entry : values.entrySet()) {
-          || value.getVersion(value.getLatestVersion()) == null);
+              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) {
+          // loop thru what's left on the main thread
-      future = future.thenApplyAsync((values) -> {
+          plugin
-        values.values().removeIf(expansion -> !expansion.isVerified());
+              .getServer()
-        return values;
+              .getScheduler()
-      });
+              .runTask(
-    }
+                  plugin,
+                  () -> {
+                    try {
+                      for (Map.Entry<String, CloudExpansion> entry : values.entrySet()) {
+                        String name = entry.getKey();
+                        CloudExpansion expansion = entry.getValue();
 
-    future = future.thenApplyAsync((values) -> {
+                        expansion.setName(name);
 
-      values.forEach((name, expansion) -> {
+                        Optional<PlaceholderExpansion> localOpt =
-        expansion.setName(name);
+                            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<PlaceholderExpansion> local = plugin.getLocalExpansionManager()
+                        cache.put(toIndexName(expansion), expansion);
-            .findExpansionByName(name);
+                      }
-        if (local.isPresent() && local.get().isRegistered()) {
+                    } catch (Throwable e) {
-          expansion.setHasExpansion(true);
+                      // ugly swallowing of every throwable, but we have to be defensive
-          expansion.setShouldUpdate(!local.get().getVersion().equals(expansion.getLatestVersion()));
+                      plugin
-        }
+                          .getLogger()
-      });
+                          .log(Level.WARNING, "Failed to download expansion information", e);
-
+                    }
-      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);
-      }
-    });
   }
 
   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);
 

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<String, PlaceholderExpansion> expansions = new HashMap<>();
+  private final Map<String, PlaceholderExpansion> 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<String> getIdentifiers() {
-    return ImmutableSet.copyOf(expansions.keySet());
+    expansionsLock.lock();
+    try {
+      return ImmutableSet.copyOf(expansions.keySet());
+    } finally {
+      expansionsLock.unlock();
+    }
   }
 
   @NotNull
   @Unmodifiable
   public Collection<PlaceholderExpansion> 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<PlaceholderExpansion> findExpansionByName(@NotNull final String name) {
-    return getExpansions().stream().filter(expansion -> name.equalsIgnoreCase(expansion.getName())).findFirst();
+    expansionsLock.lock();
+    try {
+      PlaceholderExpansion bestMatch = null;
+      for (Map.Entry<String, PlaceholderExpansion> 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> cloudExpansion = plugin.getCloudExpansionManager()
+      final Optional<CloudExpansion> cloudExpansionOptional =
-          .findCloudExpansionByName(identifier);
+          plugin.getCloudExpansionManager().findCloudExpansionByName(identifier);
-      if (cloudExpansion.isPresent()) {
+      if (cloudExpansionOptional.isPresent()) {
-        cloudExpansion.get().setHasExpansion(true);
+        CloudExpansion cloudExpansion = cloudExpansionOptional.get();
-        cloudExpansion.get().setShouldUpdate(
+        cloudExpansion.setHasExpansion(true);
-            !cloudExpansion.get().getLatestVersion().equals(expansion.getVersion()));
+        cloudExpansion.setShouldUpdate(
+            !cloudExpansion.getLatestVersion().equals(expansion.getVersion()));
       }
     }