Refactor command execution to use Locks per target instead of (effectively) one for all commands - towards #317

common/src/main/java/me/lucko/luckperms/common/commands/CommandManager.java

@@ -71,9 +71,7 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
-import java.util.concurrent.ExecutorService;
+import java.util.concurrent.CompletableFuture;
-import java.util.concurrent.Executors;
-import java.util.concurrent.Future;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
@@ -83,14 +81,11 @@ public class CommandManager {
     @Getter
     private final LuckPermsPlugin plugin;
 
-    private final ExecutorService executor;
-
     @Getter
     private final List<Command> mainCommands;
 
     public CommandManager(LuckPermsPlugin plugin) {
         this.plugin = plugin;
-        this.executor = Executors.newSingleThreadExecutor();
 
         LocaleManager locale = plugin.getLocaleManager();
 
@@ -129,8 +124,8 @@ public class CommandManager {
      * @param label  the command label used
      * @param args   the arguments provided
      */
-    public Future<CommandResult> onCommand(Sender sender, String label, List<String> args) {
+    public CompletableFuture<CommandResult> onCommand(Sender sender, String label, List<String> args) {
-        return executor.submit(() -> {
+        return CompletableFuture.supplyAsync(() -> {
             try {
                 return execute(sender, label, args);
             } catch (Throwable e) {
@@ -138,7 +133,7 @@ public class CommandManager {
                 e.printStackTrace();
                 return null;
             }
-        });
+        }, plugin.getScheduler().async());
     }
 
     @SuppressWarnings("unchecked")

common/src/main/java/me/lucko/luckperms/common/commands/abstraction/MainCommand.java

@@ -39,11 +39,14 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
+import java.util.concurrent.locks.ReentrantLock;
 import java.util.stream.Collectors;
 
-public abstract class MainCommand<T> extends Command<Void, T> {
+public abstract class MainCommand<T, I> extends Command<Void, T> {
 
-    private final int minArgs; // equals 1 if the command doesn't take a mid argument, e.g. /lp user <USER> sub-command....
+    // equals 1 if the command doesn't take a mid argument, e.g. /lp log sub-command....
+    // equals 2 if the command does take a mid argument, e.g. /lp user <USER> sub-command....
+    private final int minArgs;
 
     public MainCommand(LocalizedSpec spec, String name, int minArgs, List<Command<T, ?>> children) {
         super(spec, name, null, Predicates.alwaysFalse(), children);
@@ -84,18 +87,29 @@ public abstract class MainCommand<T> extends Command<Void, T> {
         }
 
         final String name = args.get(0);
-        T t = getTarget(name, plugin, sender);
+        I targetId = parseTarget(name, plugin, sender);
-        if (t != null) {
+        if (targetId == null) {
+            return CommandResult.LOADING_ERROR;
+        }
+
+        ReentrantLock lock = getLockForTarget(targetId);
+        lock.lock();
+        try {
+            T target = getTarget(targetId, plugin, sender);
+            if (target != null) {
                 CommandResult result;
                 try {
-                result = sub.execute(plugin, sender, t, strippedArgs, label);
+                    result = sub.execute(plugin, sender, target, strippedArgs, label);
                 } catch (CommandException e) {
                     result = CommandManager.handleException(e, sender, label, sub);
                 }
 
-            cleanup(t, plugin);
+                cleanup(target, plugin);
                 return result;
             }
+        } finally {
+            lock.unlock();
+        }
 
         return CommandResult.LOADING_ERROR;
     }
@@ -145,7 +159,11 @@ public abstract class MainCommand<T> extends Command<Void, T> {
 
     protected abstract List<String> getTargets(LuckPermsPlugin plugin);
 
-    protected abstract T getTarget(String target, LuckPermsPlugin plugin, Sender sender);
+    protected abstract I parseTarget(String target, LuckPermsPlugin plugin, Sender sender);
+
+    protected abstract ReentrantLock getLockForTarget(I target);
+
+    protected abstract T getTarget(I target, LuckPermsPlugin plugin, Sender sender);
 
     protected abstract void cleanup(T t, LuckPermsPlugin plugin);
 

common/src/main/java/me/lucko/luckperms/common/commands/abstraction/SubCommand.java

@@ -181,10 +181,12 @@ public abstract class SubCommand<T> extends Command<T, Void> {
             user.getRefreshBuffer().requestDirectly();
         }
 
+        if (!sender.isImport()) {
             InternalMessagingService messagingService = plugin.getMessagingService();
-        if (!sender.isImport() && !(messagingService instanceof NoopMessagingService) && plugin.getConfiguration().get(ConfigKeys.AUTO_PUSH_UPDATES)) {
+            if (!(messagingService instanceof NoopMessagingService) && plugin.getConfiguration().get(ConfigKeys.AUTO_PUSH_UPDATES)) {
                 messagingService.getUpdateBuffer().request();
             }
+        }
 
         if (!success) {
             Message.USER_SAVE_ERROR.send(sender);
@@ -200,10 +202,12 @@ public abstract class SubCommand<T> extends Command<T, Void> {
             plugin.getUpdateTaskBuffer().requestDirectly();
         }
 
+        if (!sender.isImport()) {
             InternalMessagingService messagingService = plugin.getMessagingService();
-        if (!sender.isImport() && !(messagingService instanceof NoopMessagingService) && plugin.getConfiguration().get(ConfigKeys.AUTO_PUSH_UPDATES)) {
+            if (!(messagingService instanceof NoopMessagingService) && plugin.getConfiguration().get(ConfigKeys.AUTO_PUSH_UPDATES)) {
                 messagingService.getUpdateBuffer().request();
             }
+        }
 
         if (!success) {
             Message.GROUP_SAVE_ERROR.send(sender);
@@ -219,10 +223,12 @@ public abstract class SubCommand<T> extends Command<T, Void> {
             plugin.getUpdateTaskBuffer().requestDirectly();
         }
 
+        if (!sender.isImport()) {
             InternalMessagingService messagingService = plugin.getMessagingService();
-        if (!sender.isImport() && !(messagingService instanceof NoopMessagingService) && plugin.getConfiguration().get(ConfigKeys.AUTO_PUSH_UPDATES)) {
+            if (!(messagingService instanceof NoopMessagingService) && plugin.getConfiguration().get(ConfigKeys.AUTO_PUSH_UPDATES)) {
                 messagingService.getUpdateBuffer().request();
             }
+        }
 
         if (!success) {
             Message.TRACK_SAVE_ERROR.send(sender);

common/src/main/java/me/lucko/luckperms/common/commands/impl/group/GroupMainCommand.java

@@ -25,6 +25,8 @@
 
 package me.lucko.luckperms.common.commands.impl.group;
 
+import com.github.benmanes.caffeine.cache.Caffeine;
+import com.github.benmanes.caffeine.cache.LoadingCache;
 import com.google.common.collect.ImmutableList;
 
 import me.lucko.luckperms.common.commands.abstraction.Command;
@@ -44,8 +46,19 @@ import me.lucko.luckperms.common.plugin.LuckPermsPlugin;
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class GroupMainCommand extends MainCommand<Group, String> {
+
+    // we use a lock per unique group
+    // this helps prevent race conditions where commands are being executed concurrently
+    // and overriding each other.
+    // it's not a great solution, but it mostly works.
+    private final LoadingCache<String, ReentrantLock> locks = Caffeine.newBuilder()
+            .expireAfterAccess(1, TimeUnit.HOURS)
+            .build(key -> new ReentrantLock());
 
-public class GroupMainCommand extends MainCommand<Group> {
     public GroupMainCommand(LocaleManager locale) {
         super(CommandSpec.GROUP.spec(locale), "Group", 2, ImmutableList.<Command<Group, ?>>builder()
                 .add(new GroupInfo(locale))
@@ -63,16 +76,19 @@ public class GroupMainCommand extends MainCommand<Group> {
         );
     }
 
+    @Override
+    protected String parseTarget(String target, LuckPermsPlugin plugin, Sender sender) {
+        return target.toLowerCase();
+    }
+
     @Override
     protected Group getTarget(String target, LuckPermsPlugin plugin, Sender sender) {
-        target = target.toLowerCase();
         if (!plugin.getStorage().loadGroup(target).join()) {
             Message.GROUP_NOT_FOUND.send(sender);
             return null;
         }
 
         Group group = plugin.getGroupManager().getIfLoaded(target);
-
         if (group == null) {
             Message.GROUP_NOT_FOUND.send(sender);
             return null;
@@ -82,6 +98,11 @@ public class GroupMainCommand extends MainCommand<Group> {
         return group;
     }
 
+    @Override
+    protected ReentrantLock getLockForTarget(String target) {
+        return locks.get(target);
+    }
+
     @Override
     protected void cleanup(Group group, LuckPermsPlugin plugin) {
 

common/src/main/java/me/lucko/luckperms/common/commands/impl/log/LogMainCommand.java

@@ -39,9 +39,12 @@ import me.lucko.luckperms.common.plugin.LuckPermsPlugin;
 import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
+import java.util.concurrent.locks.ReentrantLock;
 import java.util.stream.Collectors;
 
-public class LogMainCommand extends MainCommand<Log> {
+public class LogMainCommand extends MainCommand<Log, Object> {
+    private final ReentrantLock lock = new ReentrantLock();
+
     public LogMainCommand(LocaleManager locale) {
         super(CommandSpec.LOG.spec(locale), "Log", 1, ImmutableList.<Command<Log, ?>>builder()
                 .add(new LogRecent(locale))
@@ -55,7 +58,17 @@ public class LogMainCommand extends MainCommand<Log> {
     }
 
     @Override
-    protected Log getTarget(String target, LuckPermsPlugin plugin, Sender sender) {
+    protected ReentrantLock getLockForTarget(Object target) {
+        return lock; // all commands target the same log, so we share a lock between all "targets"
+    }
+
+    @Override
+    protected Object parseTarget(String target, LuckPermsPlugin plugin, Sender sender) {
+        return this;
+    }
+
+    @Override
+    protected Log getTarget(Object target, LuckPermsPlugin plugin, Sender sender) {
         Log log = plugin.getStorage().getLog().join();
 
         if (log == null) {
@@ -72,7 +85,7 @@ public class LogMainCommand extends MainCommand<Log> {
 
     @Override
     protected List<String> getTargets(LuckPermsPlugin plugin) {
-        return null;
+        return null; // only used for tab completion in super, and we override this method
     }
 
     @Override

common/src/main/java/me/lucko/luckperms/common/commands/impl/migration/MigrationMainCommand.java

@@ -44,8 +44,9 @@ import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
+import java.util.concurrent.locks.ReentrantLock;
 
-public class MigrationMainCommand extends MainCommand<Object> {
+public class MigrationMainCommand extends MainCommand<Object, Object> {
     private static final Map<String, String> PLUGINS = ImmutableMap.<String, String>builder()
             .put("org.anjocaido.groupmanager.GroupManager", "me.lucko.luckperms.bukkit.migration.MigrationGroupManager")
             .put("ru.tehkode.permissions.bukkit.PermissionsEx", "me.lucko.luckperms.bukkit.migration.MigrationPermissionsEx")
@@ -57,6 +58,7 @@ public class MigrationMainCommand extends MainCommand<Object> {
             .put("io.github.djxy.permissionmanager.sponge.SpongePlugin", "me.lucko.luckperms.sponge.migration.MigrationPermissionManager")
             .build();
 
+    private final ReentrantLock lock = new ReentrantLock();
     private List<Command<Object, ?>> commands = null;
     private boolean display = true;
 
@@ -109,16 +111,26 @@ public class MigrationMainCommand extends MainCommand<Object> {
         return l;
     }
 
+    @Override
+    protected ReentrantLock getLockForTarget(Object target) {
+        return lock; // share a lock between all migration commands
+    }
+
     /* Dummy */
 
     @Override
     protected List<String> getTargets(LuckPermsPlugin plugin) {
-        return Collections.emptyList();
+        return Collections.emptyList(); // only used for tab complete, we're not bothered about it for this command.
     }
 
     @Override
-    protected Object getTarget(String target, LuckPermsPlugin plugin, Sender sender) {
+    protected Object parseTarget(String target, LuckPermsPlugin plugin, Sender sender) {
-        return new Object();
+        return this; // can't return null, but we don't need a target
+    }
+
+    @Override
+    protected Object getTarget(Object target, LuckPermsPlugin plugin, Sender sender) {
+        return this; // can't return null, but we don't need a target
     }
 
     @Override

common/src/main/java/me/lucko/luckperms/common/commands/impl/track/TrackMainCommand.java

@@ -25,6 +25,8 @@
 
 package me.lucko.luckperms.common.commands.impl.track;
 
+import com.github.benmanes.caffeine.cache.Caffeine;
+import com.github.benmanes.caffeine.cache.LoadingCache;
 import com.google.common.collect.ImmutableList;
 
 import me.lucko.luckperms.common.commands.abstraction.Command;
@@ -38,8 +40,19 @@ import me.lucko.luckperms.common.plugin.LuckPermsPlugin;
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class TrackMainCommand extends MainCommand<Track, String> {
+
+    // we use a lock per unique track
+    // this helps prevent race conditions where commands are being executed concurrently
+    // and overriding each other.
+    // it's not a great solution, but it mostly works.
+    private final LoadingCache<String, ReentrantLock> locks = Caffeine.newBuilder()
+            .expireAfterAccess(1, TimeUnit.HOURS)
+            .build(key -> new ReentrantLock());
 
-public class TrackMainCommand extends MainCommand<Track> {
     public TrackMainCommand(LocaleManager locale) {
         super(CommandSpec.TRACK.spec(locale), "Track", 2, ImmutableList.<Command<Track, ?>>builder()
                 .add(new TrackInfo(locale))
@@ -53,9 +66,13 @@ public class TrackMainCommand extends MainCommand<Track> {
         );
     }
 
+    @Override
+    protected String parseTarget(String target, LuckPermsPlugin plugin, Sender sender) {
+        return target.toLowerCase();
+    }
+
     @Override
     protected Track getTarget(String target, LuckPermsPlugin plugin, Sender sender) {
-        target = target.toLowerCase();
         if (!plugin.getStorage().loadTrack(target).join()) {
             Message.TRACK_NOT_FOUND.send(sender);
             return null;
@@ -70,6 +87,11 @@ public class TrackMainCommand extends MainCommand<Track> {
         return track;
     }
 
+    @Override
+    protected ReentrantLock getLockForTarget(String target) {
+        return locks.get(target);
+    }
+
     @Override
     protected void cleanup(Track track, LuckPermsPlugin plugin) {
 

common/src/main/java/me/lucko/luckperms/common/commands/impl/user/UserMainCommand.java

@@ -25,6 +25,8 @@
 
 package me.lucko.luckperms.common.commands.impl.user;
 
+import com.github.benmanes.caffeine.cache.Caffeine;
+import com.github.benmanes.caffeine.cache.LoadingCache;
 import com.google.common.collect.ImmutableList;
 
 import me.lucko.luckperms.common.commands.abstraction.Command;
@@ -44,11 +46,23 @@ import me.lucko.luckperms.common.locale.LocaleManager;
 import me.lucko.luckperms.common.locale.Message;
 import me.lucko.luckperms.common.model.User;
 import me.lucko.luckperms.common.plugin.LuckPermsPlugin;
+import me.lucko.luckperms.common.references.UserIdentifier;
 
 import java.util.List;
 import java.util.UUID;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class UserMainCommand extends MainCommand<User, UserIdentifier> {
+
+    // we use a lock per unique user
+    // this helps prevent race conditions where commands are being executed concurrently
+    // and overriding each other.
+    // it's not a great solution, but it mostly works.
+    private final LoadingCache<UUID, ReentrantLock> locks = Caffeine.newBuilder()
+            .expireAfterAccess(1, TimeUnit.HOURS)
+            .build(key -> new ReentrantLock());
 
-public class UserMainCommand extends MainCommand<User> {
     public UserMainCommand(LocaleManager locale) {
         super(CommandSpec.USER.spec(locale), "User", 2, ImmutableList.<Command<User, ?>>builder()
                 .add(new UserInfo(locale))
@@ -66,39 +80,41 @@ public class UserMainCommand extends MainCommand<User> {
     }
 
     @Override
-    protected User getTarget(String target, LuckPermsPlugin plugin, Sender sender) {
+    protected UserIdentifier parseTarget(String target, LuckPermsPlugin plugin, Sender sender) {
-        UUID u = Util.parseUuid(target.toLowerCase());
+        UUID uuid = Util.parseUuid(target.toLowerCase());
-        if (u == null) {
+        if (uuid == null) {
             if (!DataConstraints.PLAYER_USERNAME_TEST.test(target)) {
                 Message.USER_INVALID_ENTRY.send(sender, target);
                 return null;
             }
 
-            u = plugin.getStorage().getUUID(target.toLowerCase()).join();
+            uuid = plugin.getStorage().getUUID(target.toLowerCase()).join();
-            if (u == null) {
+            if (uuid == null) {
                 if (!plugin.getConfiguration().get(ConfigKeys.USE_SERVER_UUID_CACHE)) {
                     Message.USER_NOT_FOUND.send(sender);
                     return null;
                 }
 
-                u = plugin.lookupUuid(target).orElse(null);
+                uuid = plugin.lookupUuid(target).orElse(null);
-                if (u == null) {
+                if (uuid == null) {
                     Message.USER_NOT_FOUND.send(sender);
                     return null;
                 }
             }
         }
 
-        String name = plugin.getStorage().getName(u).join();
+        String name = plugin.getStorage().getName(uuid).join();
-        if (name == null) {
+        return UserIdentifier.of(uuid, name);
-            name = "null";
     }
 
-        if (!plugin.getStorage().loadUser(u, name).join()) {
+    @Override
+    protected User getTarget(UserIdentifier target, LuckPermsPlugin plugin, Sender sender) {
+        if (!plugin.getStorage().loadUser(target.getUuid(), target.getUsername().orElse(null)).join()) {
             Message.LOADING_ERROR.send(sender);
+            return null;
         }
 
-        User user = plugin.getUserManager().getIfLoaded(u);
+        User user = plugin.getUserManager().getIfLoaded(target.getUuid());
         if (user == null) {
             Message.LOADING_ERROR.send(sender);
             return null;
@@ -108,6 +124,11 @@ public class UserMainCommand extends MainCommand<User> {
         return user;
     }
 
+    @Override
+    protected ReentrantLock getLockForTarget(UserIdentifier target) {
+        return locks.get(target.getUuid());
+    }
+
     @Override
     protected void cleanup(User user, LuckPermsPlugin plugin) {
         plugin.getUserManager().cleanup(user);