From 03f342a21ceb7e5d46f6b0afc1ee16030fe50410 Mon Sep 17 00:00:00 2001 From: Luck Date: Mon, 4 Sep 2017 20:42:21 +0100 Subject: [PATCH] Refactor command execution to use Locks per target instead of (effectively) one for all commands - towards #317 --- .../common/commands/CommandManager.java | 13 ++--- .../commands/abstraction/MainCommand.java | 44 ++++++++++++----- .../commands/abstraction/SubCommand.java | 24 +++++---- .../commands/impl/group/GroupMainCommand.java | 27 ++++++++-- .../commands/impl/log/LogMainCommand.java | 19 +++++-- .../impl/migration/MigrationMainCommand.java | 20 ++++++-- .../commands/impl/track/TrackMainCommand.java | 26 +++++++++- .../commands/impl/user/UserMainCommand.java | 49 +++++++++++++------ 8 files changed, 165 insertions(+), 57 deletions(-) diff --git a/common/src/main/java/me/lucko/luckperms/common/commands/CommandManager.java b/common/src/main/java/me/lucko/luckperms/common/commands/CommandManager.java index 599b69d2f..02a989123 100644 --- a/common/src/main/java/me/lucko/luckperms/common/commands/CommandManager.java +++ b/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.Executors; -import java.util.concurrent.Future; +import java.util.concurrent.CompletableFuture; 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 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 onCommand(Sender sender, String label, List args) { - return executor.submit(() -> { + public CompletableFuture onCommand(Sender sender, String label, List args) { + 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") diff --git a/common/src/main/java/me/lucko/luckperms/common/commands/abstraction/MainCommand.java b/common/src/main/java/me/lucko/luckperms/common/commands/abstraction/MainCommand.java index 92e55350e..5e61b0ce3 100644 --- a/common/src/main/java/me/lucko/luckperms/common/commands/abstraction/MainCommand.java +++ b/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 extends Command { +public abstract class MainCommand extends Command { - private final int minArgs; // equals 1 if the command doesn't take a mid argument, e.g. /lp 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 sub-command.... + private final int minArgs; public MainCommand(LocalizedSpec spec, String name, int minArgs, List> children) { super(spec, name, null, Predicates.alwaysFalse(), children); @@ -84,17 +87,28 @@ public abstract class MainCommand extends Command { } final String name = args.get(0); - T t = getTarget(name, plugin, sender); - if (t != null) { - CommandResult result; - try { - result = sub.execute(plugin, sender, t, strippedArgs, label); - } catch (CommandException e) { - result = CommandManager.handleException(e, sender, label, sub); - } + I targetId = parseTarget(name, plugin, sender); + if (targetId == null) { + return CommandResult.LOADING_ERROR; + } - cleanup(t, plugin); - return result; + ReentrantLock lock = getLockForTarget(targetId); + lock.lock(); + try { + T target = getTarget(targetId, plugin, sender); + if (target != null) { + CommandResult result; + try { + result = sub.execute(plugin, sender, target, strippedArgs, label); + } catch (CommandException e) { + result = CommandManager.handleException(e, sender, label, sub); + } + + cleanup(target, plugin); + return result; + } + } finally { + lock.unlock(); } return CommandResult.LOADING_ERROR; @@ -145,7 +159,11 @@ public abstract class MainCommand extends Command { protected abstract List 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); diff --git a/common/src/main/java/me/lucko/luckperms/common/commands/abstraction/SubCommand.java b/common/src/main/java/me/lucko/luckperms/common/commands/abstraction/SubCommand.java index 145f990d6..0b9abc096 100644 --- a/common/src/main/java/me/lucko/luckperms/common/commands/abstraction/SubCommand.java +++ b/common/src/main/java/me/lucko/luckperms/common/commands/abstraction/SubCommand.java @@ -181,9 +181,11 @@ public abstract class SubCommand extends Command { user.getRefreshBuffer().requestDirectly(); } - InternalMessagingService messagingService = plugin.getMessagingService(); - if (!sender.isImport() && !(messagingService instanceof NoopMessagingService) && plugin.getConfiguration().get(ConfigKeys.AUTO_PUSH_UPDATES)) { - messagingService.getUpdateBuffer().request(); + if (!sender.isImport()) { + InternalMessagingService messagingService = plugin.getMessagingService(); + if (!(messagingService instanceof NoopMessagingService) && plugin.getConfiguration().get(ConfigKeys.AUTO_PUSH_UPDATES)) { + messagingService.getUpdateBuffer().request(); + } } if (!success) { @@ -200,9 +202,11 @@ public abstract class SubCommand extends Command { plugin.getUpdateTaskBuffer().requestDirectly(); } - InternalMessagingService messagingService = plugin.getMessagingService(); - if (!sender.isImport() && !(messagingService instanceof NoopMessagingService) && plugin.getConfiguration().get(ConfigKeys.AUTO_PUSH_UPDATES)) { - messagingService.getUpdateBuffer().request(); + if (!sender.isImport()) { + InternalMessagingService messagingService = plugin.getMessagingService(); + if (!(messagingService instanceof NoopMessagingService) && plugin.getConfiguration().get(ConfigKeys.AUTO_PUSH_UPDATES)) { + messagingService.getUpdateBuffer().request(); + } } if (!success) { @@ -219,9 +223,11 @@ public abstract class SubCommand extends Command { plugin.getUpdateTaskBuffer().requestDirectly(); } - InternalMessagingService messagingService = plugin.getMessagingService(); - if (!sender.isImport() && !(messagingService instanceof NoopMessagingService) && plugin.getConfiguration().get(ConfigKeys.AUTO_PUSH_UPDATES)) { - messagingService.getUpdateBuffer().request(); + if (!sender.isImport()) { + InternalMessagingService messagingService = plugin.getMessagingService(); + if (!(messagingService instanceof NoopMessagingService) && plugin.getConfiguration().get(ConfigKeys.AUTO_PUSH_UPDATES)) { + messagingService.getUpdateBuffer().request(); + } } if (!success) { diff --git a/common/src/main/java/me/lucko/luckperms/common/commands/impl/group/GroupMainCommand.java b/common/src/main/java/me/lucko/luckperms/common/commands/impl/group/GroupMainCommand.java index 12424a879..3e64e839c 100644 --- a/common/src/main/java/me/lucko/luckperms/common/commands/impl/group/GroupMainCommand.java +++ b/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 { + + // 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 locks = Caffeine.newBuilder() + .expireAfterAccess(1, TimeUnit.HOURS) + .build(key -> new ReentrantLock()); -public class GroupMainCommand extends MainCommand { public GroupMainCommand(LocaleManager locale) { super(CommandSpec.GROUP.spec(locale), "Group", 2, ImmutableList.>builder() .add(new GroupInfo(locale)) @@ -63,16 +76,19 @@ public class GroupMainCommand extends MainCommand { ); } + @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 { return group; } + @Override + protected ReentrantLock getLockForTarget(String target) { + return locks.get(target); + } + @Override protected void cleanup(Group group, LuckPermsPlugin plugin) { diff --git a/common/src/main/java/me/lucko/luckperms/common/commands/impl/log/LogMainCommand.java b/common/src/main/java/me/lucko/luckperms/common/commands/impl/log/LogMainCommand.java index 71eaf6614..09f2fb912 100644 --- a/common/src/main/java/me/lucko/luckperms/common/commands/impl/log/LogMainCommand.java +++ b/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 { +public class LogMainCommand extends MainCommand { + private final ReentrantLock lock = new ReentrantLock(); + public LogMainCommand(LocaleManager locale) { super(CommandSpec.LOG.spec(locale), "Log", 1, ImmutableList.>builder() .add(new LogRecent(locale)) @@ -55,7 +58,17 @@ public class LogMainCommand extends MainCommand { } @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 { @Override protected List getTargets(LuckPermsPlugin plugin) { - return null; + return null; // only used for tab completion in super, and we override this method } @Override diff --git a/common/src/main/java/me/lucko/luckperms/common/commands/impl/migration/MigrationMainCommand.java b/common/src/main/java/me/lucko/luckperms/common/commands/impl/migration/MigrationMainCommand.java index 3c2ba9c38..765367ec8 100644 --- a/common/src/main/java/me/lucko/luckperms/common/commands/impl/migration/MigrationMainCommand.java +++ b/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 { +public class MigrationMainCommand extends MainCommand { private static final Map PLUGINS = ImmutableMap.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 { .put("io.github.djxy.permissionmanager.sponge.SpongePlugin", "me.lucko.luckperms.sponge.migration.MigrationPermissionManager") .build(); + private final ReentrantLock lock = new ReentrantLock(); private List> commands = null; private boolean display = true; @@ -109,16 +111,26 @@ public class MigrationMainCommand extends MainCommand { return l; } + @Override + protected ReentrantLock getLockForTarget(Object target) { + return lock; // share a lock between all migration commands + } + /* Dummy */ @Override protected List 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) { - return new Object(); + protected Object parseTarget(String target, LuckPermsPlugin plugin, Sender sender) { + 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 diff --git a/common/src/main/java/me/lucko/luckperms/common/commands/impl/track/TrackMainCommand.java b/common/src/main/java/me/lucko/luckperms/common/commands/impl/track/TrackMainCommand.java index df8f3d4b4..cb973a996 100644 --- a/common/src/main/java/me/lucko/luckperms/common/commands/impl/track/TrackMainCommand.java +++ b/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 { + + // 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 locks = Caffeine.newBuilder() + .expireAfterAccess(1, TimeUnit.HOURS) + .build(key -> new ReentrantLock()); -public class TrackMainCommand extends MainCommand { public TrackMainCommand(LocaleManager locale) { super(CommandSpec.TRACK.spec(locale), "Track", 2, ImmutableList.>builder() .add(new TrackInfo(locale)) @@ -53,9 +66,13 @@ public class TrackMainCommand extends MainCommand { ); } + @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 { return track; } + @Override + protected ReentrantLock getLockForTarget(String target) { + return locks.get(target); + } + @Override protected void cleanup(Track track, LuckPermsPlugin plugin) { diff --git a/common/src/main/java/me/lucko/luckperms/common/commands/impl/user/UserMainCommand.java b/common/src/main/java/me/lucko/luckperms/common/commands/impl/user/UserMainCommand.java index c070a0d94..f488fca1d 100644 --- a/common/src/main/java/me/lucko/luckperms/common/commands/impl/user/UserMainCommand.java +++ b/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 { + + // 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 locks = Caffeine.newBuilder() + .expireAfterAccess(1, TimeUnit.HOURS) + .build(key -> new ReentrantLock()); -public class UserMainCommand extends MainCommand { public UserMainCommand(LocaleManager locale) { super(CommandSpec.USER.spec(locale), "User", 2, ImmutableList.>builder() .add(new UserInfo(locale)) @@ -66,39 +80,41 @@ public class UserMainCommand extends MainCommand { } @Override - protected User getTarget(String target, LuckPermsPlugin plugin, Sender sender) { - UUID u = Util.parseUuid(target.toLowerCase()); - if (u == null) { + protected UserIdentifier parseTarget(String target, LuckPermsPlugin plugin, Sender sender) { + UUID uuid = Util.parseUuid(target.toLowerCase()); + 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(); - if (u == null) { + uuid = plugin.getStorage().getUUID(target.toLowerCase()).join(); + 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); - if (u == null) { + uuid = plugin.lookupUuid(target).orElse(null); + if (uuid == null) { Message.USER_NOT_FOUND.send(sender); return null; } } } - String name = plugin.getStorage().getName(u).join(); - if (name == null) { - name = "null"; - } + String name = plugin.getStorage().getName(uuid).join(); + return UserIdentifier.of(uuid, name); + } - 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 { 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);