Fix permissions not being removed on Sponge

bukkit/src/main/java/me/lucko/luckperms/users/BukkitUserManager.java

@@ -44,23 +44,18 @@ public class BukkitUserManager extends UserManager {
     }
 
     @Override
-    public void unload(User user) {
+    public void preUnload(User user) {
-        if (user != null) {
+        if (user instanceof BukkitUser) {
+            BukkitUser u = (BukkitUser) user;
 
-            if (user instanceof BukkitUser) {
+            if (u.getAttachment() != null) {
-                BukkitUser u = (BukkitUser) user;
+                Player player = plugin.getServer().getPlayer(plugin.getUuidCache().getExternalUUID(u.getUuid()));
 
-                if (u.getAttachment() != null) {
+                if (player != null) {
-                    Player player = plugin.getServer().getPlayer(plugin.getUuidCache().getExternalUUID(u.getUuid()));
+                    player.removeAttachment(u.getAttachment());
-
-                    if (player != null) {
-                        player.removeAttachment(u.getAttachment());
-                    }
-                    u.setAttachment(null);
                 }
+                u.setAttachment(null);
             }
-
-            getAll().remove(user.getUuid());
         }
     }
 

bungee/src/main/java/me/lucko/luckperms/users/BungeeUserManager.java

@@ -34,15 +34,6 @@ public class BungeeUserManager extends UserManager {
         this.plugin = plugin;
     }
 
-    @Override
-    public void unload(User user) {
-        if (user != null) {
-            // Cannot clear the ProxiedPlayer's permission map, they're leaving so that will get GCed anyway
-            // Calling getPermissions.clear() throws an UnsupportedOperationException
-            getAll().remove(user.getUuid());
-        }
-    }
-
     @Override
     public void cleanup(User user) {
         if (plugin.getProxy().getPlayer(plugin.getUuidCache().getExternalUUID(user.getUuid())) == null) {

common/src/main/java/me/lucko/luckperms/tracks/TrackManager.java

@@ -35,7 +35,7 @@ public class TrackManager extends AbstractManager<String, Track> {
      * @return a set of tracks that the groups could be a member of
      */
     public Set<Track> getApplicableTracks(String group) {
-        return objects.values().stream()
+        return getAll().values().stream()
                 .filter(t -> t.containsGroup(group))
                 .collect(Collectors.toSet());
     }

common/src/main/java/me/lucko/luckperms/users/UserManager.java

@@ -45,7 +45,7 @@ public abstract class UserManager extends AbstractManager<UUID, User> {
     @SuppressWarnings("OptionalGetWithoutIsPresent")
     public User get(String name) {
         try {
-            return objects.values().stream()
+            return getAll().values().stream()
                     .filter(u -> u.getName().equalsIgnoreCase(name))
                     .limit(1).findAny().get();
         } catch (NoSuchElementException e) {
@@ -54,9 +54,8 @@ public abstract class UserManager extends AbstractManager<UUID, User> {
     }
 
     @Override
-    public void set(User u) {
+    public void preSet(User u) {
         giveDefaultIfNeeded(u, true);
-        super.set(u);
     }
 
     @Override

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

@@ -22,8 +22,10 @@
 
 package me.lucko.luckperms.utils;
 
+import com.google.common.collect.ImmutableMap;
+
+import java.util.HashMap;
 import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
 
 /**
  * An abstract manager class
@@ -31,10 +33,12 @@ import java.util.concurrent.ConcurrentHashMap;
  * @param <T> the class this manager is "managing"
  */
 public abstract class AbstractManager<I, T extends Identifiable<I>> {
-    protected final Map<I, T> objects = new ConcurrentHashMap<>();
+    private final Map<I, T> objects = new HashMap<>();
 
-    public Map<I, T> getAll() {
+    public final Map<I, T> getAll() {
-        return objects;
+        synchronized (objects) {
+            return ImmutableMap.copyOf(objects);
+        }
     }
 
     /**
@@ -42,28 +46,39 @@ public abstract class AbstractManager<I, T extends Identifiable<I>> {
      * @param id The id to search by
      * @return a {@link T} object if the object is loaded, returns null if the object is not loaded
      */
-    public T get(I id) {
+    public final T get(I id) {
-        return objects.get(id);
+        synchronized (objects) {
+            return objects.get(id);
+        }
     }
 
     /**
      * Add a object to the loaded objects map
      * @param t The object to add
      */
-    public void set(T t) {
+    public final void set(T t) {
-        objects.put(t.getId(), t);
+        preSet(t);
+        synchronized (objects) {
+            objects.put(t.getId(), t);
+        }
+    }
+
+    public void preSet(T t) {
+
     }
 
     /**
      * Updates (or sets if the object wasn't already loaded) an object in the objects map
      * @param t The object to update or set
      */
-    public void updateOrSet(T t) {
+    public final void updateOrSet(T t) {
-        if (!isLoaded(t.getId())) {
+        synchronized (objects) {
-            // The object isn't already loaded
+            if (!isLoaded(t.getId())) {
-            set(t);
+                // The object isn't already loaded
-        } else {
+                set(t);
-            copy(t, objects.get(t.getId()));
+            } else {
+                copy(t, objects.get(t.getId()));
+            }
         }
     }
 
@@ -74,25 +89,36 @@ public abstract class AbstractManager<I, T extends Identifiable<I>> {
      * @param id The id of the object
      * @return true if the object is loaded
      */
-    public boolean isLoaded(I id) {
+    public final boolean isLoaded(I id) {
-        return objects.containsKey(id);
+        synchronized (objects) {
+            return objects.containsKey(id);
+        }
     }
 
     /**
      * Removes and unloads the object from the manager
      * @param t The object to unload
      */
-    public void unload(T t) {
+    public final void unload(T t) {
         if (t != null) {
-            objects.remove(t.getId());
+            preUnload(t);
+            synchronized (objects) {
+                objects.remove(t.getId());
+            }
         }
     }
 
+    public void preUnload(T t) {
+
+    }
+
     /**
      * Unloads all objects from the manager
      */
-    public void unloadAll() {
+    public final void unloadAll() {
-        objects.clear();
+        synchronized (objects) {
+            objects.clear();
+        }
     }
 
     /**

sponge/src/main/java/me/lucko/luckperms/SpongeListener.java

@@ -22,7 +22,6 @@
 
 package me.lucko.luckperms;
 
-import me.lucko.luckperms.api.sponge.LuckPermsSubject;
 import me.lucko.luckperms.constants.Message;
 import me.lucko.luckperms.users.User;
 import me.lucko.luckperms.utils.AbstractListener;
@@ -31,8 +30,6 @@ import org.spongepowered.api.event.network.ClientConnectionEvent;
 import org.spongepowered.api.profile.GameProfile;
 import org.spongepowered.api.text.serializer.TextSerializers;
 
-import java.util.Iterator;
-
 @SuppressWarnings("WeakerAccess")
 public class SpongeListener extends AbstractListener {
     private final LPSpongePlugin plugin;
@@ -78,12 +75,5 @@ public class SpongeListener extends AbstractListener {
     @Listener
     public void onClientLeave(ClientConnectionEvent.Disconnect e) {
         onLeave(e.getTargetEntity().getUniqueId());
-        Iterator<LuckPermsSubject> iterator = plugin.getService().getUserSubjects().getCache().iterator();
-        while (iterator.hasNext()) {
-            LuckPermsSubject subject = iterator.next();
-            if (subject.getIdentifier().equalsIgnoreCase(e.getTargetEntity().getUniqueId().toString())) {
-                iterator.remove();
-            }
-        }
     }
 }

sponge/src/main/java/me/lucko/luckperms/api/sponge/LuckPermsSubject.java

@@ -52,13 +52,17 @@ import static me.lucko.luckperms.utils.ArgumentChecker.unescapeCharacters;
 
 @EqualsAndHashCode(of = {"holder"})
 public class LuckPermsSubject implements Subject {
+    public static Subject wrapHolder(PermissionHolder holder, LuckPermsService service) {
+        return new LuckPermsSubject(holder, service);
+    }
+
     @Getter
     private final PermissionHolder holder;
     private final EnduringData enduringData;
     private final TransientData transientData;
     private final LuckPermsService service;
 
-    public LuckPermsSubject(PermissionHolder holder, LuckPermsService service) {
+    private LuckPermsSubject(PermissionHolder holder, LuckPermsService service) {
         this.holder = holder;
         this.enduringData = new EnduringData(this, service, holder);
         this.transientData = new TransientData(service, holder);
@@ -77,13 +81,13 @@ public class LuckPermsSubject implements Subject {
 
     @Override
     public String getIdentifier() {
-        return enduringData.getHolder().getObjectName();
+        return holder.getObjectName();
     }
 
     @Override
     public Optional<CommandSource> getCommandSource() {
-        if (enduringData.getHolder() instanceof User) {
+        if (holder instanceof User) {
-            final UUID uuid = ((User) enduringData.getHolder()).getUuid();
+            final UUID uuid = ((User) holder).getUuid();
 
             Optional<Player> p = Sponge.getServer().getPlayer(uuid);
             if (p.isPresent()) {
@@ -96,7 +100,7 @@ public class LuckPermsSubject implements Subject {
 
     @Override
     public SubjectCollection getContainingCollection() {
-        if (enduringData.getHolder() instanceof Group) {
+        if (holder instanceof Group) {
             return service.getGroupSubjects();
         } else {
             return service.getUserSubjects();
@@ -130,18 +134,16 @@ public class LuckPermsSubject implements Subject {
             context.put(c.getKey(), c.getValue());
         }
 
-        me.lucko.luckperms.api.Tristate t = enduringData.getHolder().inheritsPermission(new me.lucko.luckperms.utils.Node.Builder(node).withExtraContext(context).build());
+        switch (holder.inheritsPermission(new me.lucko.luckperms.utils.Node.Builder(node).withExtraContext(context).build())) {
-        if (t == me.lucko.luckperms.api.Tristate.UNDEFINED) {
+            case UNDEFINED:
-            return Tristate.UNDEFINED;
+                return Tristate.UNDEFINED;
+            case TRUE:
+                return Tristate.TRUE;
+            case FALSE:
+                return Tristate.FALSE;
+            default:
+                return null;
         }
-        if (t == me.lucko.luckperms.api.Tristate.TRUE) {
-            return Tristate.TRUE;
-        }
-        if (t == me.lucko.luckperms.api.Tristate.FALSE) {
-            return Tristate.FALSE;
-        }
-
-        return null;
     }
 
     @Override

sponge/src/main/java/me/lucko/luckperms/api/sponge/collections/GroupCollection.java

@@ -27,7 +27,6 @@ import lombok.NonNull;
 import me.lucko.luckperms.api.sponge.LuckPermsService;
 import me.lucko.luckperms.api.sponge.LuckPermsSubject;
 import me.lucko.luckperms.api.sponge.simple.SimpleSubject;
-import me.lucko.luckperms.core.PermissionHolder;
 import me.lucko.luckperms.groups.GroupManager;
 import org.spongepowered.api.service.context.Context;
 import org.spongepowered.api.service.permission.PermissionService;
@@ -35,17 +34,14 @@ import org.spongepowered.api.service.permission.Subject;
 import org.spongepowered.api.service.permission.SubjectCollection;
 import org.spongepowered.api.service.permission.SubjectData;
 
-import java.util.AbstractMap;
 import java.util.Map;
 import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
 import java.util.stream.Collectors;
 
 @AllArgsConstructor
 public class GroupCollection implements SubjectCollection {
     private final LuckPermsService service;
     private final GroupManager manager;
-    private final Set<LuckPermsSubject> cache = ConcurrentHashMap.newKeySet();
 
     @Override
     public String getIdentifier() {
@@ -55,16 +51,7 @@ public class GroupCollection implements SubjectCollection {
     @Override
     public Subject get(@NonNull String id) {
         if (manager.isLoaded(id)) {
-            PermissionHolder holder = manager.get(id);
+            return LuckPermsSubject.wrapHolder(manager.get(id), service);
-            for (LuckPermsSubject subject : cache) {
-                if (subject.getHolder().getObjectName().equalsIgnoreCase(holder.getObjectName())) {
-                    return subject;
-                }
-            }
-
-            LuckPermsSubject subject = new LuckPermsSubject(manager.get(id), service);
-            cache.add(subject);
-            return subject;
         }
 
         return new SimpleSubject(id, service, this);
@@ -78,7 +65,7 @@ public class GroupCollection implements SubjectCollection {
     @Override
     public Iterable<Subject> getAllSubjects() {
         return manager.getAll().values().stream()
-                .map(u -> new LuckPermsSubject(u, service))
+                .map(u -> LuckPermsSubject.wrapHolder(u, service))
                 .collect(Collectors.toList());
     }
 
@@ -90,10 +77,9 @@ public class GroupCollection implements SubjectCollection {
     @Override
     public Map<Subject, Boolean> getAllWithPermission(@NonNull Set<Context> contexts, @NonNull String node) {
         return manager.getAll().values().stream()
-                .map(u -> new LuckPermsSubject(u, service))
+                .map(u -> LuckPermsSubject.wrapHolder(u, service))
                 .filter(sub -> sub.hasPermission(contexts, node))
-                .map(sub -> new AbstractMap.SimpleEntry<Subject, Boolean>(sub, sub.getPermissionValue(contexts, node).asBoolean()))
+                .collect(Collectors.toMap(sub -> sub, sub -> sub.getPermissionValue(contexts, node).asBoolean()));
-                .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
     }
 
     @Override

sponge/src/main/java/me/lucko/luckperms/api/sponge/collections/UserCollection.java

@@ -23,7 +23,6 @@
 package me.lucko.luckperms.api.sponge.collections;
 
 import lombok.AllArgsConstructor;
-import lombok.Getter;
 import lombok.NonNull;
 import me.lucko.luckperms.api.sponge.LuckPermsService;
 import me.lucko.luckperms.api.sponge.LuckPermsSubject;
@@ -37,11 +36,9 @@ import org.spongepowered.api.service.permission.Subject;
 import org.spongepowered.api.service.permission.SubjectCollection;
 import org.spongepowered.api.service.permission.SubjectData;
 
-import java.util.AbstractMap;
 import java.util.Map;
 import java.util.Set;
 import java.util.UUID;
-import java.util.concurrent.ConcurrentHashMap;
 import java.util.stream.Collectors;
 
 @AllArgsConstructor
@@ -49,9 +46,6 @@ public class UserCollection implements SubjectCollection {
     private final LuckPermsService service;
     private final UserManager manager;
 
-    @Getter
-    private final Set<LuckPermsSubject> cache = ConcurrentHashMap.newKeySet();
-
     @Override
     public String getIdentifier() {
         return PermissionService.SUBJECTS_USER;
@@ -74,15 +68,7 @@ public class UserCollection implements SubjectCollection {
         }
 
         if (holder != null) {
-            for (LuckPermsSubject subject : cache) {
+            return LuckPermsSubject.wrapHolder(holder, service);
-                if (subject.getHolder().getObjectName().equalsIgnoreCase(holder.getObjectName())) {
-                    return subject;
-                }
-            }
-
-            LuckPermsSubject subject = new LuckPermsSubject(holder, service);
-            cache.add(subject);
-            return subject;
         }
 
         service.getPlugin().getLog().warn("Couldn't get subject for: " + id);
@@ -106,7 +92,7 @@ public class UserCollection implements SubjectCollection {
     @Override
     public Iterable<Subject> getAllSubjects() {
         return manager.getAll().values().stream()
-                .map(u -> new LuckPermsSubject(u, service))
+                .map(u -> LuckPermsSubject.wrapHolder(u, service))
                 .collect(Collectors.toList());
     }
 
@@ -118,10 +104,9 @@ public class UserCollection implements SubjectCollection {
     @Override
     public Map<Subject, Boolean> getAllWithPermission(@NonNull Set<Context> contexts, @NonNull String node) {
         return manager.getAll().values().stream()
-                .map(u -> new LuckPermsSubject(u, service))
+                .map(u -> LuckPermsSubject.wrapHolder(u, service))
                 .filter(sub -> sub.hasPermission(contexts, node))
-                .map(sub -> new AbstractMap.SimpleEntry<Subject, Boolean>(sub, sub.getPermissionValue(contexts, node).asBoolean()))
+                .collect(Collectors.toMap(sub -> sub, sub -> sub.getPermissionValue(contexts, node).asBoolean()));
-                .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
     }
 
     @Override

sponge/src/main/java/me/lucko/luckperms/users/SpongeUserManager.java

@@ -34,16 +34,9 @@ public class SpongeUserManager extends UserManager {
         this.plugin = plugin;
     }
 
-    @Override
-    public void unload(User user) {
-        if (user != null) {
-            getAll().remove(user.getUuid());
-        }
-    }
-
     @Override
     public void cleanup(User user) {
-        if (plugin.getGame().getServer().getPlayer(plugin.getUuidCache().getExternalUUID(user.getUuid())).isPresent()) {
+        if (!plugin.getGame().getServer().getPlayer(plugin.getUuidCache().getExternalUUID(user.getUuid())).isPresent()) {
             unload(user);
         }
     }