Avoid running full "update tasks" unless they're absolutely needed. Process Vault API "set" requests immediately instead of in the background

bukkit/src/main/java/me/lucko/luckperms/bukkit/vault/VaultChatHook.java

@@ -50,11 +50,10 @@ import java.util.UUID;
 /**
  * An implementation of the Vault {@link Chat} API using LuckPerms.
  *
- * Methods which change the state of data objects are likely to return immediately.
- *
  * LuckPerms is a multithreaded permissions plugin, and some actions require considerable
  * time to execute. (database queries, re-population of caches, etc) In these cases, the
- * methods will return immediately and the change will be executed asynchronously.
+ * operations required to make the edit apply will be processed immediately, but the process
+ * of saving the change to the plugin storage will happen in the background.
  *
  * Methods that have to query data from the database will throw exceptions when called
  * from the main thread. Users of the Vault API expect these methods to be "main thread friendly",
@@ -251,27 +250,25 @@ public class VaultChatHook extends AbstractVaultChat {
             logMsg("#setChatMeta: %s - %s - %s - %s", holder.getFriendlyName(), type, value, world);
         }
 
-        this.permissionHook.getExecutor().execute(() -> {
+        // remove all prefixes/suffixes directly set on the user/group
-            // remove all prefixes/suffixes directly set on the user/group
+        holder.removeIf(type::matches);
-            holder.removeIf(type::matches);
 
-            if (value == null) {
+        if (value == null) {
-                this.permissionHook.holderSave(holder);
-                return;
-            }
-
-            // find the max inherited priority & add 10
-            MetaAccumulator metaAccumulator = holder.accumulateMeta(null, createContextForWorldSet(world));
-            metaAccumulator.complete();
-            int priority = metaAccumulator.getChatMeta(type).keySet().stream().mapToInt(e -> e).max().orElse(0) + 10;
-
-            Node.Builder chatMetaNode = NodeFactory.buildChatMetaNode(type, priority, value);
-            chatMetaNode.setServer(this.permissionHook.getVaultServer());
-            chatMetaNode.setWorld(world);
-
-            holder.setPermission(chatMetaNode.build());
             this.permissionHook.holderSave(holder);
-        });
+            return;
+        }
+
+        // find the max inherited priority & add 10
+        MetaAccumulator metaAccumulator = holder.accumulateMeta(null, createContextForWorldSet(world));
+        metaAccumulator.complete();
+        int priority = metaAccumulator.getChatMeta(type).keySet().stream().mapToInt(e -> e).max().orElse(0) + 10;
+
+        Node.Builder chatMetaNode = NodeFactory.buildChatMetaNode(type, priority, value);
+        chatMetaNode.setServer(this.permissionHook.getVaultServer());
+        chatMetaNode.setWorld(world);
+
+        holder.setPermission(chatMetaNode.build()); // assume success
+        this.permissionHook.holderSave(holder);
     }
 
     private void setMeta(PermissionHolder holder, String key, Object value, String world) {
@@ -279,27 +276,25 @@ public class VaultChatHook extends AbstractVaultChat {
             logMsg("#setMeta: %s - %s - %s - %s", holder.getFriendlyName(), key, value, world);
         }
 
-        this.permissionHook.getExecutor().execute(() -> {
+        holder.removeIf(n -> n.isMeta() && n.getMeta().getKey().equals(key));
-            holder.removeIf(n -> n.isMeta() && n.getMeta().getKey().equals(key));
 
-            if (value == null) {
+        if (value == null) {
-                this.permissionHook.holderSave(holder);
-                return;
-            }
-
-            Node.Builder metaNode;
-            if (key.equalsIgnoreCase(NodeTypes.PREFIX_KEY) || key.equalsIgnoreCase(NodeTypes.SUFFIX_KEY)) {
-                metaNode = NodeFactory.buildChatMetaNode(ChatMetaType.valueOf(key.toUpperCase()), 100, value.toString());
-            } else {
-                metaNode = NodeFactory.buildMetaNode(key, value.toString());
-            }
-
-            metaNode.setServer(this.permissionHook.getVaultServer());
-            metaNode.setWorld(world);
-
-            holder.setPermission(metaNode.build());
             this.permissionHook.holderSave(holder);
-        });
+            return;
+        }
+
+        Node.Builder metaNode;
+        if (key.equalsIgnoreCase(NodeTypes.PREFIX_KEY) || key.equalsIgnoreCase(NodeTypes.SUFFIX_KEY)) {
+            metaNode = NodeFactory.buildChatMetaNode(ChatMetaType.valueOf(key.toUpperCase()), 100, value.toString());
+        } else {
+            metaNode = NodeFactory.buildMetaNode(key, value.toString());
+        }
+
+        metaNode.setServer(this.permissionHook.getVaultServer());
+        metaNode.setWorld(world);
+
+        holder.setPermission(metaNode.build()); // assume success
+        this.permissionHook.holderSave(holder);
     }
 
     private Contexts createContextForWorldSet(String world) {

bukkit/src/main/java/me/lucko/luckperms/bukkit/vault/VaultPermissionHook.java

@@ -51,17 +51,14 @@ import java.util.Arrays;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.UUID;
-import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Executors;
 
 /**
  * An implementation of the Vault {@link Permission} API using LuckPerms.
  *
- * Methods which change the state of data objects are likely to return immediately.
- *
  * LuckPerms is a multithreaded permissions plugin, and some actions require considerable
  * time to execute. (database queries, re-population of caches, etc) In these cases, the
- * methods will return immediately and the change will be executed asynchronously.
+ * operations required to make the edit apply will be processed immediately, but the process
+ * of saving the change to the plugin storage will happen in the background.
  *
  * Methods that have to query data from the database will throw exceptions when called
  * from the main thread. Users of the Vault API expect these methods to be "main thread friendly",
@@ -73,12 +70,8 @@ public class VaultPermissionHook extends AbstractVaultPermission {
     // the plugin instance
     private final LPBukkitPlugin plugin;
 
-    // an executor for Vault modifications.
-    private final ExecutorService executor;
-
     public VaultPermissionHook(LPBukkitPlugin plugin) {
         this.plugin = plugin;
-        this.executor = Executors.newSingleThreadExecutor();
         this.worldMappingFunction = world -> isIgnoreWorld() ? null : world;
     }
 
@@ -86,10 +79,6 @@ public class VaultPermissionHook extends AbstractVaultPermission {
         return this.plugin;
     }
 
-    public ExecutorService getExecutor() {
-        return this.executor;
-    }
-
     @Override
     public String getName() {
         return "LuckPerms";
@@ -119,9 +108,9 @@ public class VaultPermissionHook extends AbstractVaultPermission {
         }
 
         // lookup a username from the database
-        UUID uuid = plugin.getStorage().getPlayerUuid(player.toLowerCase()).join();
+        UUID uuid = this.plugin.getStorage().getPlayerUuid(player.toLowerCase()).join();
         if (uuid == null) {
-            uuid = plugin.getBootstrap().lookupUuid(player).orElse(null);
+            uuid = this.plugin.getBootstrap().lookupUuid(player).orElse(null);
         }
 
         // unable to find a user, throw an exception
@@ -186,8 +175,7 @@ public class VaultPermissionHook extends AbstractVaultPermission {
         Objects.requireNonNull(permission, "permission");
 
         User user = lookupUser(uuid);
-        holderAddPermission(user, permission, world);
+        return holderAddPermission(user, permission, world);
-        return true;
     }
 
     @Override
@@ -196,8 +184,7 @@ public class VaultPermissionHook extends AbstractVaultPermission {
         Objects.requireNonNull(permission, "permission");
 
         User user = lookupUser(uuid);
-        holderRemovePermission(user, permission, world);
+        return holderRemovePermission(user, permission, world);
-        return true;
     }
 
     @Override
@@ -295,8 +282,7 @@ public class VaultPermissionHook extends AbstractVaultPermission {
             return false;
         }
 
-        holderAddPermission(group, permission, world);
+        return holderAddPermission(group, permission, world);
-        return true;
     }
 
     @Override
@@ -309,8 +295,7 @@ public class VaultPermissionHook extends AbstractVaultPermission {
             return false;
         }
 
-        holderRemovePermission(group, permission, world);
+        return holderRemovePermission(group, permission, world);
-        return true;
     }
 
     // utility methods for getting user and group instances
@@ -380,7 +365,7 @@ public class VaultPermissionHook extends AbstractVaultPermission {
 
     // utility methods for modifying the state of PermissionHolders
 
-    private void holderAddPermission(PermissionHolder holder, String permission, String world) {
+    private boolean holderAddPermission(PermissionHolder holder, String permission, String world) {
         Objects.requireNonNull(permission, "permission is null");
         Preconditions.checkArgument(!permission.isEmpty(), "permission is an empty string");
 
@@ -388,14 +373,13 @@ public class VaultPermissionHook extends AbstractVaultPermission {
             logMsg("#holderAddPermission: %s - %s - %s", holder.getFriendlyName(), permission, world);
         }
 
-        this.executor.execute(() -> {
+        if (holder.setPermission(NodeFactory.make(permission, true, getVaultServer(), world)).asBoolean()) {
-            if (holder.setPermission(NodeFactory.make(permission, true, getVaultServer(), world)).asBoolean()) {
+            return holderSave(holder);
-                holderSave(holder);
+        }
-            }
+        return false;
-        });
     }
 
-    private void holderRemovePermission(PermissionHolder holder, String permission, String world) {
+    private boolean holderRemovePermission(PermissionHolder holder, String permission, String world) {
         Objects.requireNonNull(permission, "permission is null");
         Preconditions.checkArgument(!permission.isEmpty(), "permission is an empty string");
 
@@ -403,22 +387,32 @@ public class VaultPermissionHook extends AbstractVaultPermission {
             logMsg("#holderRemovePermission: %s - %s - %s", holder.getFriendlyName(), permission, world);
         }
 
-        this.executor.execute(() -> {
+        if (holder.unsetPermission(NodeFactory.make(permission, getVaultServer(), world)).asBoolean()) {
-            if (holder.unsetPermission(NodeFactory.make(permission, getVaultServer(), world)).asBoolean()) {
+            return holderSave(holder);
-                holderSave(holder);
+        }
-            }
+        return false;
-        });
     }
 
-    void holderSave(PermissionHolder holder) {
+    boolean holderSave(PermissionHolder holder) {
         if (holder.getType().isUser()) {
             User u = (User) holder;
+
+            // we don't need to join this call - the save operation
+            // can happen in the background.
             this.plugin.getStorage().saveUser(u);
-        }
+        } else if (holder.getType().isGroup()) {
-        if (holder.getType().isGroup()) {
             Group g = (Group) holder;
-            this.plugin.getStorage().saveGroup(g).thenRunAsync(() -> this.plugin.getUpdateTaskBuffer().request(), this.plugin.getBootstrap().getScheduler().async());
+
+            // invalidate caches - they have potentially been affected by
+            // this change.
+            this.plugin.getGroupManager().invalidateAllGroupCaches();
+            this.plugin.getUserManager().invalidateAllUserCaches();
+
+            // we don't need to join this call - the save operation
+            // can happen in the background.
+            this.plugin.getStorage().saveGroup(g);
         }
+        return true;
     }
 
     // helper methods to just pull values from the config.

common/src/main/java/me/lucko/luckperms/common/command/utils/StorageAssistant.java

@@ -130,7 +130,8 @@ public final class StorageAssistant {
             return;
         }
 
-        plugin.getUpdateTaskBuffer().requestDirectly();
+        plugin.getGroupManager().invalidateAllGroupCaches();
+        plugin.getUserManager().invalidateAllUserCaches();
 
         Optional<InternalMessagingService> messagingService = plugin.getMessagingService();
         if (messagingService.isPresent() && plugin.getConfiguration().get(ConfigKeys.AUTO_PUSH_UPDATES)) {
@@ -154,7 +155,8 @@ public final class StorageAssistant {
             return;
         }
 
-        plugin.getUpdateTaskBuffer().requestDirectly();
+        plugin.getGroupManager().invalidateAllGroupCaches();
+        plugin.getUserManager().invalidateAllUserCaches();
 
         Optional<InternalMessagingService> messagingService = plugin.getMessagingService();
         if (messagingService.isPresent() && plugin.getConfiguration().get(ConfigKeys.AUTO_PUSH_UPDATES)) {

common/src/main/java/me/lucko/luckperms/common/managers/group/AbstractGroupManager.java

@@ -27,6 +27,7 @@ package me.lucko.luckperms.common.managers.group;
 
 import me.lucko.luckperms.common.managers.AbstractManager;
 import me.lucko.luckperms.common.model.Group;
+import me.lucko.luckperms.common.model.PermissionHolder;
 
 import java.util.Optional;
 
@@ -63,4 +64,9 @@ public abstract class AbstractGroupManager<T extends Group> extends AbstractMana
     protected String sanitizeIdentifier(String s) {
         return s.toLowerCase();
     }
+
+    @Override
+    public void invalidateAllGroupCaches() {
+        getAll().values().forEach(PermissionHolder::invalidateCachedData);
+    }
 }

common/src/main/java/me/lucko/luckperms/common/managers/group/GroupManager.java

@@ -38,4 +38,9 @@ public interface GroupManager<T extends Group> extends Manager<String, Group, T>
      */
     T getByDisplayName(String name);
 
+    /**
+     * Invalidates the cached data for *loaded* groups.
+     */
+    void invalidateAllGroupCaches();
+
 }

common/src/main/java/me/lucko/luckperms/common/managers/user/AbstractUserManager.java

@@ -29,6 +29,7 @@ import me.lucko.luckperms.api.Node;
 import me.lucko.luckperms.api.context.ImmutableContextSet;
 import me.lucko.luckperms.common.config.ConfigKeys;
 import me.lucko.luckperms.common.managers.AbstractManager;
+import me.lucko.luckperms.common.model.PermissionHolder;
 import me.lucko.luckperms.common.model.User;
 import me.lucko.luckperms.common.model.UserIdentifier;
 import me.lucko.luckperms.common.node.factory.NodeFactory;
@@ -153,6 +154,11 @@ public abstract class AbstractUserManager<T extends User> extends AbstractManage
         );
     }
 
+    @Override
+    public void invalidateAllUserCaches() {
+        getAll().values().forEach(PermissionHolder::invalidateCachedData);
+    }
+
     /**
      * Check whether the user's state indicates that they should be persisted to storage.
      *

common/src/main/java/me/lucko/luckperms/common/managers/user/UserManager.java

@@ -80,8 +80,13 @@ public interface UserManager<T extends User> extends Manager<UserIdentifier, Use
     void cleanup(User user);
 
     /**
-     * Reloads the data of all online users
+     * Reloads the data of all *online* users
      */
     CompletableFuture<Void> updateAllUsers();
 
+    /**
+     * Invalidates the cached data for *loaded* users.
+     */
+    void invalidateAllUserCaches();
+
 }

common/src/main/java/me/lucko/luckperms/common/tasks/ExpireTemporaryTask.java

@@ -60,7 +60,8 @@ public class ExpireTemporaryTask implements Runnable {
         }
 
         if (groupChanges) {
-            this.plugin.getUpdateTaskBuffer().request();
+            this.plugin.getGroupManager().invalidateAllGroupCaches();
+            this.plugin.getUserManager().invalidateAllUserCaches();
         }
     }
 

common/src/main/java/me/lucko/luckperms/common/tasks/UpdateTask.java

@@ -70,6 +70,7 @@ public class UpdateTask implements Runnable {
         this.plugin.getStorage().loadAllTracks().join();
 
         // Refresh all online users.
+        this.plugin.getUserManager().invalidateAllUserCaches();
         CompletableFuture<Void> userUpdateFut = this.plugin.getUserManager().updateAllUsers();
         if (!this.initialUpdate) {
             userUpdateFut.join();

sponge/src/main/java/me/lucko/luckperms/sponge/service/internal/HolderSubjectData.java

@@ -451,7 +451,9 @@ public class HolderSubjectData implements LPSubjectData {
         if (this.type == NodeMapType.TRANSIENT) {
             // don't bother saving to primary storage. just refresh
             if (t.getType().isGroup()) {
-                return this.service.getPlugin().getUpdateTaskBuffer().request();
+                this.service.getPlugin().getGroupManager().invalidateAllGroupCaches();
+                this.service.getPlugin().getUserManager().invalidateAllUserCaches();
+                return CompletableFuture.completedFuture(null);
             }
         }
 
@@ -461,8 +463,9 @@ public class HolderSubjectData implements LPSubjectData {
             return this.service.getPlugin().getStorage().saveUser(user);
         } else {
             Group group = ((Group) t);
-            return this.service.getPlugin().getStorage().saveGroup(group)
+            this.service.getPlugin().getGroupManager().invalidateAllGroupCaches();
-                    .thenCompose(v -> this.service.getPlugin().getUpdateTaskBuffer().request());
+            this.service.getPlugin().getUserManager().invalidateAllUserCaches();
+            return this.service.getPlugin().getStorage().saveGroup(group);
         }
     }
 }