Properly cleanup in the case of a reload - towards #100

bukkit/src/main/java/me/lucko/luckperms/bukkit/BukkitListener.java

@@ -117,7 +117,7 @@ class BukkitListener extends AbstractListener implements Listener {
         plugin.getWorldCalculator().getWorldCache().remove(internal);
 
         // Remove the custom permissible
-        Injector.unInject(player);
+        Injector.unInject(player, true);
 
         // Handle auto op
         if (plugin.getConfiguration().isAutoOp()) {

bukkit/src/main/java/me/lucko/luckperms/bukkit/LPBukkitPlugin.java

@@ -34,8 +34,10 @@ import me.lucko.luckperms.api.PlatformType;
 import me.lucko.luckperms.api.context.ContextSet;
 import me.lucko.luckperms.api.context.MutableContextSet;
 import me.lucko.luckperms.bukkit.calculators.AutoOPListener;
+import me.lucko.luckperms.bukkit.inject.Injector;
 import me.lucko.luckperms.bukkit.model.ChildPermissionProvider;
 import me.lucko.luckperms.bukkit.model.DefaultsProvider;
+import me.lucko.luckperms.bukkit.model.LPPermissible;
 import me.lucko.luckperms.bukkit.vault.VaultHook;
 import me.lucko.luckperms.common.LuckPermsPlugin;
 import me.lucko.luckperms.common.api.ApiProvider;
@@ -72,6 +74,7 @@ import me.lucko.luckperms.common.utils.PermissionCache;
 import org.bukkit.World;
 import org.bukkit.command.PluginCommand;
 import org.bukkit.entity.Player;
+import org.bukkit.event.HandlerList;
 import org.bukkit.permissions.PermissionDefault;
 import org.bukkit.plugin.PluginManager;
 import org.bukkit.plugin.ServicePriority;
@@ -92,7 +95,7 @@ import java.util.stream.Collectors;
 
 @Getter
 public class LPBukkitPlugin extends JavaPlugin implements LuckPermsPlugin {
-    private final Set<UUID> ignoringLogs = ConcurrentHashMap.newKeySet();
+    private Set<UUID> ignoringLogs;
     private Executor syncExecutor;
     private Executor asyncExecutor;
     private VaultHook vaultHook = null;
@@ -103,6 +106,7 @@ public class LPBukkitPlugin extends JavaPlugin implements LuckPermsPlugin {
     private Storage storage;
     private RedisMessaging redisMessaging = null;
     private UuidCache uuidCache;
+    private BukkitListener listener;
     private ApiProvider apiProvider;
     private Logger log;
     private Importer importer;
@@ -127,6 +131,7 @@ public class LPBukkitPlugin extends JavaPlugin implements LuckPermsPlugin {
         Executor bukkitAsyncExecutor = r -> getServer().getScheduler().runTaskAsynchronously(this, r);
 
         log = LogFactory.wrap(getLogger());
+        ignoringLogs = ConcurrentHashMap.newKeySet();
         debugHandler = new DebugHandler(bukkitAsyncExecutor, getVersion());
         senderFactory = new BukkitSenderFactory(this);
         permissionCache = new PermissionCache(bukkitAsyncExecutor);
@@ -168,7 +173,8 @@ public class LPBukkitPlugin extends JavaPlugin implements LuckPermsPlugin {
 
         // register events
         PluginManager pm = getServer().getPluginManager();
-        pm.registerEvents(new BukkitListener(this), this);
+        listener = new BukkitListener(this);
+        pm.registerEvents(listener, this);
 
         // initialise datastore
         storage = StorageFactory.getInstance(this, StorageType.H2);
@@ -250,7 +256,7 @@ public class LPBukkitPlugin extends JavaPlugin implements LuckPermsPlugin {
         int mins = getConfiguration().getSyncTime();
         if (mins > 0) {
             long ticks = mins * 60 * 20;
-            getServer().getScheduler().runTaskTimerAsynchronously(this, () -> updateTaskBuffer.request(), 20L, ticks);
+            getServer().getScheduler().runTaskTimerAsynchronously(this, () -> updateTaskBuffer.request(), 40L, ticks);
         }
 
         // run an update instantly.
@@ -271,6 +277,24 @@ public class LPBukkitPlugin extends JavaPlugin implements LuckPermsPlugin {
             asyncExecutor = bukkitAsyncExecutor;
         });
 
+        // Load any online users (in the case of a reload)
+        for (Player player : getServer().getOnlinePlayers()) {
+            doAsync(() -> {
+                listener.onAsyncLogin(player.getUniqueId(), player.getName());
+                User user = getUserManager().get(getUuidCache().getUUID(player.getUniqueId()));
+                if (user != null) {
+                    doSync(() -> {
+                        try {
+                            LPPermissible lpPermissible = new LPPermissible(player, user, this);
+                            Injector.inject(player, lpPermissible);
+                        } catch (Throwable t) {
+                            t.printStackTrace();
+                        }
+                    });
+                }
+            });
+        }
+
         started = true;
         getLog().info("Successfully loaded.");
     }
@@ -278,6 +302,24 @@ public class LPBukkitPlugin extends JavaPlugin implements LuckPermsPlugin {
     @Override
     public void onDisable() {
         started = false;
+
+        defaultsProvider.close();
+        permissionCache.setShutdown(true);
+        debugHandler.setShutdown(true);
+
+        for (Player player : getServer().getOnlinePlayers()) {
+            Injector.unInject(player, false);
+            if (getConfiguration().isAutoOp()) {
+                player.setOp(false);
+            }
+
+            final User user = getUserManager().get(getUuidCache().getUUID(player.getUniqueId()));
+            if (user != null) {
+                user.unregisterData();
+                getUserManager().unload(user);
+            }
+        }
+
         getLog().info("Closing datastore...");
         storage.shutdown();
 
@@ -293,6 +335,38 @@ public class LPBukkitPlugin extends JavaPlugin implements LuckPermsPlugin {
         if (vaultHook != null) {
             vaultHook.unhook(this);
         }
+
+        // Bukkit will do this again when #onDisable completes, but we do it early to prevent NPEs elsewhere.
+        getServer().getScheduler().cancelTasks(this);
+        HandlerList.unregisterAll(this);
+
+        // Null everything
+        ignoringLogs = null;
+        syncExecutor = null;
+        asyncExecutor = null;
+        vaultHook = null;
+        configuration = null;
+        userManager = null;
+        groupManager = null;
+        trackManager = null;
+        storage = null;
+        redisMessaging = null;
+        uuidCache = null;
+        listener = null;
+        apiProvider = null;
+        log = null;
+        importer = null;
+        defaultsProvider = null;
+        childPermissionProvider = null;
+        localeManager = null;
+        cachedStateManager = null;
+        contextManager = null;
+        worldCalculator = null;
+        calculatorFactory = null;
+        updateTaskBuffer = null;
+        debugHandler = null;
+        senderFactory = null;
+        permissionCache = null;
     }
 
     public void tryVaultHook(boolean force) {

bukkit/src/main/java/me/lucko/luckperms/bukkit/inject/Injector.java

@@ -28,9 +28,11 @@ import me.lucko.luckperms.bukkit.model.LPPermissible;
 
 import org.bukkit.Bukkit;
 import org.bukkit.entity.Player;
-import org.bukkit.permissions.Permissible;
+import org.bukkit.permissions.PermissibleBase;
+import org.bukkit.permissions.PermissionAttachment;
 
 import java.lang.reflect.Field;
+import java.util.List;
 import java.util.Map;
 import java.util.UUID;
 import java.util.concurrent.ConcurrentHashMap;
@@ -41,21 +43,42 @@ import java.util.concurrent.ConcurrentHashMap;
 @UtilityClass
 public class Injector {
     private static final Map<UUID, LPPermissible> INJECTED_PERMISSIBLES = new ConcurrentHashMap<>();
+
     private static Field HUMAN_ENTITY_FIELD;
+    private static Field PERMISSIBLEBASE_ATTACHMENTS;
 
     static {
         try {
             HUMAN_ENTITY_FIELD = Class.forName(getVersionedClassName("entity.CraftHumanEntity")).getDeclaredField("perm");
             HUMAN_ENTITY_FIELD.setAccessible(true);
+
+            PERMISSIBLEBASE_ATTACHMENTS = PermissibleBase.class.getDeclaredField("attachments");
+            PERMISSIBLEBASE_ATTACHMENTS.setAccessible(true);
+
         } catch (Exception e) {
             e.printStackTrace();
         }
     }
 
-    public static boolean inject(Player player, LPPermissible permissible) {
+    public static boolean inject(Player player, LPPermissible lpPermissible) {
         try {
-            HUMAN_ENTITY_FIELD.set(player, permissible);
+            PermissibleBase existing = (PermissibleBase) HUMAN_ENTITY_FIELD.get(player);
-            INJECTED_PERMISSIBLES.put(player.getUniqueId(), permissible);
+            if (existing instanceof LPPermissible) {
+                // uh oh
+                throw new IllegalStateException();
+            }
+
+            // Move attachments over from the old permissible.
+            List<PermissionAttachment> attachments = (List<PermissionAttachment>) PERMISSIBLEBASE_ATTACHMENTS.get(existing);
+            lpPermissible.addAttachments(attachments);
+            attachments.clear();
+            existing.clearPermissions();
+
+            lpPermissible.recalculatePermissions();
+            lpPermissible.setOldPermissible(existing);
+
+            HUMAN_ENTITY_FIELD.set(player, lpPermissible);
+            INJECTED_PERMISSIBLES.put(player.getUniqueId(), lpPermissible);
             return true;
         } catch (Exception e) {
             e.printStackTrace();
@@ -63,14 +86,28 @@ public class Injector {
         }
     }
 
-    public static boolean unInject(Player player) {
+    public static boolean unInject(Player player, boolean dummy) {
         try {
-            Permissible permissible = (Permissible) HUMAN_ENTITY_FIELD.get(player);
+            PermissibleBase permissible = (PermissibleBase) HUMAN_ENTITY_FIELD.get(player);
             if (permissible instanceof LPPermissible) {
-                /* The player is most likely leaving. Bukkit will attempt to call #clearPermissions, so we cannot set to null.
+                if (dummy) {
-                   However, there's no need to re-inject a real PermissibleBase, so we just inject a dummy instead.
-                   This saves tick time, pointlessly recalculating defaults when the instance will never be used. */
                     HUMAN_ENTITY_FIELD.set(player, new DummyPermissibleBase());
+                } else {
+                    LPPermissible lpp = ((LPPermissible) permissible);
+                    List<PermissionAttachment> attachments = lpp.getAttachments();
+
+                    PermissibleBase newPb = lpp.getOldPermissible();
+                    if (newPb == null) {
+                        newPb = new PermissibleBase(player);
+                    }
+
+                    List<PermissionAttachment> newAttachments = (List<PermissionAttachment>) PERMISSIBLEBASE_ATTACHMENTS.get(newPb);
+                    newAttachments.addAll(attachments);
+                    attachments.clear();
+                    lpp.clearPermissions();
+
+                    HUMAN_ENTITY_FIELD.set(player, newPb);
+                }
             }
             INJECTED_PERMISSIBLES.remove(player.getUniqueId());
             return true;

bukkit/src/main/java/me/lucko/luckperms/bukkit/model/DefaultsProvider.java

@@ -111,6 +111,11 @@ public class DefaultsProvider {
         nonOpDefaults = ImmutableMap.copyOf(builder);
     }
 
+    public void close() {
+        unregisterDefaults(opDefaults, opDummy);
+        unregisterDefaults(nonOpDefaults, nonOpDummy);
+    }
+
     public Tristate hasDefault(String permission, boolean isOp) {
         Map<String, Boolean> map = isOp ? opDefaults : nonOpDefaults;
 

bukkit/src/main/java/me/lucko/luckperms/bukkit/model/LPPermissible.java

@@ -24,6 +24,7 @@ package me.lucko.luckperms.bukkit.model;
 
 import lombok.Getter;
 import lombok.NonNull;
+import lombok.Setter;
 
 import me.lucko.luckperms.api.Contexts;
 import me.lucko.luckperms.api.Tristate;
@@ -39,12 +40,13 @@ import org.bukkit.permissions.PermissionAttachmentInfo;
 import org.bukkit.permissions.PermissionRemovedExecutor;
 import org.bukkit.plugin.Plugin;
 
-import java.util.HashMap;
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.logging.Level;
 import java.util.stream.Collectors;
 
@@ -61,10 +63,15 @@ public class LPPermissible extends PermissibleBase {
 
     private final LPBukkitPlugin plugin;
 
+    @Getter
+    @Setter
+    private PermissibleBase oldPermissible = null;
+
     // Attachment stuff.
     @Getter
-    private final Map<String, PermissionAttachmentInfo> attachmentPermissions = new HashMap<>();
+    private final Map<String, PermissionAttachmentInfo> attachmentPermissions = new ConcurrentHashMap<>();
-    private final List<PermissionAttachment> attachments = new LinkedList<>();
+    @Getter
+    private final List<PermissionAttachment> attachments = Collections.synchronizedList(new LinkedList<>());
 
     public LPPermissible(@NonNull Player parent, User user, LPBukkitPlugin plugin) {
         super(parent);
@@ -75,6 +82,12 @@ public class LPPermissible extends PermissibleBase {
         recalculatePermissions();
     }
 
+    public void addAttachments(List<PermissionAttachment> attachments) {
+        for (PermissionAttachment attachment : attachments) {
+            this.attachments.add(attachment);
+        }
+    }
+
     public Contexts calculateContexts() {
         return new Contexts(
                 plugin.getContextManager().getApplicableContext(parent),

common/src/main/java/me/lucko/luckperms/common/utils/AbstractListener.java

@@ -39,7 +39,7 @@ import java.util.UUID;
 public class AbstractListener {
     private final LuckPermsPlugin plugin;
 
-    protected void onAsyncLogin(UUID u, String username) {
+    public void onAsyncLogin(UUID u, String username) {
         final long startTime = System.currentTimeMillis();
 
         final UuidCache cache = plugin.getUuidCache();

common/src/main/java/me/lucko/luckperms/common/utils/DebugHandler.java

@@ -58,6 +58,9 @@ public class DebugHandler {
     private final Map<Receiver, List<String>> listeners;
     private final Queue<Data> queue;
 
+    @Setter
+    private boolean shutdown = false;
+
     public DebugHandler(Executor executor, String pluginVersion) {
         this.pluginVersion = "v" + pluginVersion;
         listeners = new ConcurrentHashMap<>();
@@ -69,6 +72,10 @@ public class DebugHandler {
                     handleOutput(e.getChecked(), e.getNode(), e.getValue());
                 }
 
+                if (shutdown) {
+                    return;
+                }
+
                 try {
                     Thread.sleep(200);
                 } catch (InterruptedException ignored) {}

common/src/main/java/me/lucko/luckperms/common/utils/PermissionCache.java

@@ -24,6 +24,7 @@ package me.lucko.luckperms.common.utils;
 
 import lombok.Getter;
 import lombok.NonNull;
+import lombok.Setter;
 
 import com.google.common.base.Splitter;
 
@@ -41,6 +42,9 @@ public class PermissionCache {
     private final Node rootNode;
     private final Queue<String> queue;
 
+    @Setter
+    private boolean shutdown = false;
+
     public PermissionCache(Executor executor) {
         rootNode = new Node();
         queue = new ConcurrentLinkedQueue<>();
@@ -51,10 +55,13 @@ public class PermissionCache {
                     insert(e.toLowerCase());
                 }
 
+                if (shutdown) {
+                    return;
+                }
+
                 try {
                     Thread.sleep(5000);
-                } catch (InterruptedException ignored) {
+                } catch (InterruptedException ignored) {}
-                }
             }
         });
     }