From 2fe5a1130c8ff164e0a381a8e16f3097ec396ee9 Mon Sep 17 00:00:00 2001 From: zml Date: Sun, 21 Jun 2015 15:57:31 -0700 Subject: [PATCH] Revert "Rework locking to try to reduce lag. Related to #1824" This reverts commit 6fbeb4901d7c3c36f01fac51208f856f95d29515. This caused issues for some users, so it is being reverted in the interest of functionality. --- .../backends/caching/CachingData.java | 104 +++++------------ .../backends/caching/CachingGroupData.java | 3 +- .../backends/caching/CachingUserData.java | 5 +- .../backends/file/FileBackend.java | 103 +++-------------- .../permissions/backends/file/FileData.java | 108 +++++++++++++----- .../permissions/backends/sql/SQLBackend.java | 5 +- 6 files changed, 135 insertions(+), 193 deletions(-) diff --git a/src/main/java/ru/tehkode/permissions/backends/caching/CachingData.java b/src/main/java/ru/tehkode/permissions/backends/caching/CachingData.java index 25bc67920..37c8940b4 100644 --- a/src/main/java/ru/tehkode/permissions/backends/caching/CachingData.java +++ b/src/main/java/ru/tehkode/permissions/backends/caching/CachingData.java @@ -9,111 +9,73 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.Executor; -import java.util.concurrent.locks.ReadWriteLock; /** * Data backend implementing a simple cache */ public abstract class CachingData implements PermissionsData { private final Executor executor; - protected final ReadWriteLock lock; + protected final Object lock; private Map> permissions; private Map> options; private Map> parents; private volatile Set worlds; - public CachingData(Executor executor, ReadWriteLock lock) { + public CachingData(Executor executor, Object lock) { this.executor = executor; this.lock = lock; } - protected void executeWrite(final Runnable run) { + protected void execute(final Runnable run) { executor.execute(new Runnable() { @Override public void run() { - lock.writeLock().lock(); - try { + synchronized (lock) { run.run(); - } finally { - lock.readLock().lock(); - try { - lock.writeLock().unlock(); - getBackingData().save(); - } finally { - lock.readLock().unlock(); - } } } }); } - protected void executeRead(final Runnable run) { - executor.execute(new Runnable() { - @Override - public void run() { - lock.readLock().lock(); - try { - run.run(); - } finally { - lock.readLock().unlock(); - } - } - }); - } protected abstract PermissionsData getBackingData(); protected void loadPermissions() { - lock.readLock().lock(); - try { + synchronized (lock) { this.permissions = new HashMap<>(getBackingData().getPermissionsMap()); - } finally { - lock.readLock().unlock(); } } protected void loadOptions() { - lock.readLock().lock(); - try { + synchronized (lock) { this.options = new HashMap<>(); for (Map.Entry> e : getBackingData().getOptionsMap().entrySet()) { this.options.put(e.getKey(), new HashMap<>(e.getValue())); } - } finally { - lock.readLock().unlock(); } } protected void loadInheritance() { - lock.readLock().lock(); - try { + synchronized (lock) { this.parents = new HashMap<>(getBackingData().getParentsMap()); - } finally { - lock.readLock().unlock(); } } protected void clearCache() { - lock.writeLock().lock(); - try { + synchronized (lock) { permissions = null; options = null; parents = null; clearWorldsCache(); - } finally { - lock.writeLock().unlock(); } } @Override public void load() { - lock.writeLock().lock(); - try { + synchronized (lock) { getBackingData().load(); loadInheritance(); loadOptions(); loadPermissions(); - } finally { - lock.writeLock().unlock(); } } @@ -137,7 +99,7 @@ public void setPermissions(List permissions, final String worldName) { loadPermissions(); } final List safePermissions = new ArrayList<>(permissions); - executeWrite(new Runnable() { + execute(new Runnable() { @Override public void run() { clearWorldsCache(); @@ -164,11 +126,8 @@ public Map> getPermissionsMap() { public Set getWorlds() { Set worlds = this.worlds; if (worlds == null) { - lock.readLock().lock(); - try { + synchronized (lock) { this.worlds = worlds = getBackingData().getWorlds(); - } finally { - lock.readLock().unlock(); } } return worlds; @@ -195,26 +154,26 @@ public void setOption(final String option, final String value, final String worl if (options == null) { loadOptions(); } - executeWrite(new Runnable() { + execute(new Runnable() { @Override public void run() { - if (options != null) { - Map optionsMap = options.get(world); - if (optionsMap == null) { - // TODO Concurrentify - optionsMap = new HashMap<>(); - options.put(world, optionsMap); - clearWorldsCache(); - } - if (value == null) { - optionsMap.remove(option); - } else { - optionsMap.put(option, value); - } - } getBackingData().setOption(option, value, world); } }); + if (options != null) { + Map optionsMap = options.get(world); + if (optionsMap == null) { + // TODO Concurrentify + optionsMap = new HashMap<>(); + options.put(world, optionsMap); + clearWorldsCache(); + } + if (value == null) { + optionsMap.remove(option); + } else { + optionsMap.put(option, value); + } + } } @Override @@ -254,13 +213,13 @@ public void setParents(final List rawParents, final String worldName) { loadInheritance(); } final List safeParents = new ArrayList<>(rawParents); - executeWrite(new Runnable() { + execute(new Runnable() { @Override public void run() { - parents.put(worldName, Collections.unmodifiableList(safeParents)); getBackingData().setParents(safeParents, worldName); } }); + this.parents.put(worldName, Collections.unmodifiableList(safeParents)); } @Override @@ -270,7 +229,7 @@ public boolean isVirtual() { @Override public void save() { - executeRead(new Runnable() { + execute(new Runnable() { @Override public void run() { getBackingData().save(); @@ -280,12 +239,9 @@ public void run() { @Override public void remove() { - lock.writeLock().lock(); - try { + synchronized (lock) { getBackingData().remove(); clearCache(); - } finally { - lock.writeLock().unlock(); } } diff --git a/src/main/java/ru/tehkode/permissions/backends/caching/CachingGroupData.java b/src/main/java/ru/tehkode/permissions/backends/caching/CachingGroupData.java index 49185ce07..894685f3e 100644 --- a/src/main/java/ru/tehkode/permissions/backends/caching/CachingGroupData.java +++ b/src/main/java/ru/tehkode/permissions/backends/caching/CachingGroupData.java @@ -3,14 +3,13 @@ import ru.tehkode.permissions.PermissionsGroupData; import java.util.concurrent.Executor; -import java.util.concurrent.locks.ReadWriteLock; /** * Cached data for groups */ public class CachingGroupData extends CachingData implements PermissionsGroupData { private final PermissionsGroupData backingData; - public CachingGroupData(PermissionsGroupData backingData, Executor executor, ReadWriteLock lock) { + public CachingGroupData(PermissionsGroupData backingData, Executor executor, Object lock) { super(executor, lock); this.backingData = backingData; } diff --git a/src/main/java/ru/tehkode/permissions/backends/caching/CachingUserData.java b/src/main/java/ru/tehkode/permissions/backends/caching/CachingUserData.java index 882fdeb6e..09068c275 100644 --- a/src/main/java/ru/tehkode/permissions/backends/caching/CachingUserData.java +++ b/src/main/java/ru/tehkode/permissions/backends/caching/CachingUserData.java @@ -3,14 +3,13 @@ import ru.tehkode.permissions.PermissionsUserData; import java.util.concurrent.Executor; -import java.util.concurrent.locks.ReadWriteLock; /** * User data using a cache. */ public class CachingUserData extends CachingData implements PermissionsUserData { private final PermissionsUserData userData; - public CachingUserData(PermissionsUserData userData, Executor executor, ReadWriteLock lock) { + public CachingUserData(PermissionsUserData userData, Executor executor, Object lock) { super(executor, lock); this.userData = userData; } @@ -22,7 +21,7 @@ protected PermissionsUserData getBackingData() { @Override public boolean setIdentifier(final String identifier) { - executeWrite(new Runnable() { + execute(new Runnable() { @Override public void run() { getBackingData().setIdentifier(identifier); diff --git a/src/main/java/ru/tehkode/permissions/backends/file/FileBackend.java b/src/main/java/ru/tehkode/permissions/backends/file/FileBackend.java index 30a382b04..de9c5e507 100644 --- a/src/main/java/ru/tehkode/permissions/backends/file/FileBackend.java +++ b/src/main/java/ru/tehkode/permissions/backends/file/FileBackend.java @@ -34,8 +34,6 @@ import java.io.Writer; import java.util.*; import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.locks.ReadWriteLock; -import java.util.concurrent.locks.ReentrantReadWriteLock; /** * @author code @@ -45,7 +43,7 @@ public class FileBackend extends PermissionBackend { public FileConfig permissions; public File permissionsFile; private final Map> worldInheritanceCache = new ConcurrentHashMap<>(); - private final ReadWriteLock lock = new ReentrantReadWriteLock(); + private final Object lock = new Object(); public FileBackend(PermissionManager manager, ConfigurationSection config) throws PermissionBackendException { super(manager, config); @@ -125,23 +123,17 @@ private void singleWorld(ConfigurationSection section) { @Override public int getSchemaVersion() { - lock.readLock().lock(); - try { + synchronized (lock) { return this.permissions.getInt("schema-version", -1); - } finally { - lock.readLock().unlock(); } } @Override protected void setSchemaVersion(int version) { - lock.writeLock().lock(); - try { + synchronized (lock) { this.permissions.set("schema-version", version); - } finally { - lock.writeLock().unlock(); + save(); } - save(); } @Override @@ -200,77 +192,34 @@ public void run() { }); } - private ConfigurationSection getNode(String basePath, String entityName) { - if (permissions.isLowerCased(basePath)) { - entityName = entityName.toLowerCase(); - } - String nodePath = FileBackend.buildPath(basePath, entityName); - lock.readLock().lock(); - try { - - - ConfigurationSection entityNode = this.permissions.getConfigurationSection(nodePath); - - if (entityNode != null) { - return entityNode; - } - - if (!permissions.isLowerCased(basePath)) { - ConfigurationSection users = this.permissions.getConfigurationSection(basePath); - - if (users != null) { - for (Map.Entry entry : users.getValues(false).entrySet()) { - if (entry.getKey().equalsIgnoreCase(entityName) - && entry.getValue() instanceof ConfigurationSection) { - return (ConfigurationSection) entry.getValue(); - } - } - } - } - } finally { - lock.readLock().unlock(); - } - - lock.writeLock().lock(); - try { - ConfigurationSection section = this.permissions.createSection(nodePath); - this.permissions.set(nodePath, null); - return section; - } finally { - lock.writeLock().unlock(); - } - } - @Override public PermissionsUserData getUserData(String userName) { - ConfigurationSection section = getNode("users", userName); - final CachingUserData data = new CachingUserData(new FileData(section, "group"), getExecutor(), lock); - data.load(); - return data; + synchronized (lock) { + final CachingUserData data = new CachingUserData(new FileData("users", userName, this.permissions, "group"), getExecutor(), lock); + data.load(); + return data; + } } @Override public PermissionsGroupData getGroupData(String groupName) { - ConfigurationSection section = getNode("groups", groupName); - final CachingGroupData data = new CachingGroupData(new FileData(section, "inheritance"), getExecutor(), lock); - data.load(); - return data; + synchronized (lock) { + final CachingGroupData data = new CachingGroupData(new FileData("groups", groupName, this.permissions, "inheritance"), getExecutor(), lock); + data.load(); + return data; + } } @Override public boolean hasUser(String userName) { - lock.readLock().lock(); - try { + synchronized (lock) { return this.permissions.isConfigurationSection(buildPath("users", userName.toLowerCase())); - } finally { - lock.readLock().unlock(); } } @Override public boolean hasGroup(String group) { - lock.readLock().lock(); - try { + synchronized (lock) { if (this.permissions.isConfigurationSection(buildPath("groups", group))) { return true; } @@ -285,26 +234,20 @@ public boolean hasGroup(String group) { } return false; - } finally { - lock.readLock().unlock(); } } @Override public Collection getUserIdentifiers() { - lock.readLock().lock(); - try { + synchronized (lock) { ConfigurationSection users = this.permissions.getConfigurationSection("users"); return users != null ? users.getKeys(false) : Collections.emptyList(); - } finally { - lock.readLock().unlock(); } } @Override public Collection getUserNames() { - lock.readLock().lock(); - try { + synchronized (lock) { ConfigurationSection users = this.permissions.getConfigurationSection("users"); if (users == null) { @@ -324,19 +267,14 @@ public Collection getUserNames() { } } return Collections.unmodifiableSet(userNames); - } finally { - lock.readLock().unlock(); } } @Override public Collection getGroupNames() { - lock.readLock().lock(); - try { + synchronized (lock) { ConfigurationSection groups = this.permissions.getConfigurationSection("groups"); return groups != null ? groups.getKeys(false) : Collections.emptySet(); - } finally { - lock.readLock().unlock(); } } @@ -437,13 +375,10 @@ public void writeContents(Writer writer) throws IOException { } public void save() { - lock.readLock().lock(); try { this.permissions.save(); } catch (IOException e) { getManager().getLogger().severe("Error while saving permissions file: " + e.getMessage()); - } finally { - lock.readLock().unlock(); } } } diff --git a/src/main/java/ru/tehkode/permissions/backends/file/FileData.java b/src/main/java/ru/tehkode/permissions/backends/file/FileData.java index 6ae18160a..6b84f8b44 100644 --- a/src/main/java/ru/tehkode/permissions/backends/file/FileData.java +++ b/src/main/java/ru/tehkode/permissions/backends/file/FileData.java @@ -11,47 +11,98 @@ public class FileData implements PermissionsUserData, PermissionsGroupData { protected transient final FileConfig config; + private String nodePath, entityName; + private final String basePath; private ConfigurationSection node; protected boolean virtual = true; private final String parentPath; - public FileData(ConfigurationSection node, String parentPath) { - this.config = (FileConfig) node.getRoot(); - this.node = node; - this.virtual = node.getParent().getConfigurationSection(node.getName()) == null; + public FileData(String basePath, String name, FileConfig config, String parentPath) { + this.config = config; + this.basePath = basePath; + this.node = findNode(name); this.parentPath = parentPath; } - @Override - public String getIdentifier() { - return node.getName(); - } + private ConfigurationSection findExistingNode(String entityName, boolean set) { + if (config.isLowerCased(basePath)) { + entityName = entityName.toLowerCase(); + } + String nodePath = FileBackend.buildPath(basePath, entityName); - @Override - public boolean setIdentifier(String identifier) { - String caseCorrectedIdentifier = config.isLowerCased(node.getParent().getCurrentPath()) ? identifier.toLowerCase() : identifier; - ConfigurationSection existing = node.getParent().getConfigurationSection(identifier); - if (existing == null && config.isLowerCased(node.getParent().getCurrentPath())) { - ConfigurationSection users = node.getParent(); + ConfigurationSection entityNode = this.config.getConfigurationSection(nodePath); + + if (entityNode != null) { + this.virtual = false; + if (set) { + this.nodePath = nodePath; + this.entityName = entityName; + } + return entityNode; + } + + if (!config.isLowerCased(basePath)) { + ConfigurationSection users = this.config.getConfigurationSection(basePath); if (users != null) { for (Map.Entry entry : users.getValues(false).entrySet()) { - if (entry.getKey().equalsIgnoreCase(caseCorrectedIdentifier) + if (entry.getKey().equalsIgnoreCase(entityName) && entry.getValue() instanceof ConfigurationSection) { - existing = (ConfigurationSection) entry.getValue(); + if (set) { + this.nodePath = FileBackend.buildPath(basePath, entry.getKey()); + this.entityName = entry.getKey(); + } + return (ConfigurationSection) entry.getValue(); } } } } - if (existing != null) { + return null; + } + + private ConfigurationSection findNode(String entityName) { + if (config.isLowerCased(basePath)) { + entityName = entityName.toLowerCase(); + } + ConfigurationSection section = findExistingNode(entityName, true); + if (section != null) { + return section; + } + + // Silly workaround for empty nodes + this.nodePath = FileBackend.buildPath(basePath, entityName); + section = this.config.createSection(nodePath); + this.entityName = entityName; + this.config.set(nodePath, null); + this.virtual = true; // Make sure + + return section; + + } + + @Override + public String getIdentifier() { + return entityName; + } + + @Override + public boolean setIdentifier(String identifier) { + ConfigurationSection section = findExistingNode(identifier, false); + if (section != null) { return false; } - ConfigurationSection oldNode = node; - this.node = oldNode.getParent().createSection(caseCorrectedIdentifier, node.getValues(false)); - oldNode.getParent().set(oldNode.getName(), null); - if (this.isVirtual()) { - node.getParent().set(node.getName(), null); + String caseCorrectedIdentifier = config.isLowerCased(basePath) ? identifier.toLowerCase() : identifier; + String oldNodePath = this.nodePath; + this.nodePath = FileBackend.buildPath(basePath, caseCorrectedIdentifier); + this.node = this.config.createSection(nodePath, node.getValues(false)); + this.entityName = identifier; + this.config.set(oldNodePath, null); + if (!this.isVirtual()) { + this.config.set(nodePath, node); + this.save(); + } else { + this.config.set(nodePath, null); } return true; } @@ -69,6 +120,7 @@ public List getPermissions(String worldName) { @Override public void setPermissions(List permissions, String worldName) { this.node.set(formatPath(worldName, "permissions"), permissions == null ? null : new ArrayList<>(permissions)); + save(); } @Override @@ -114,6 +166,7 @@ public String getOption(String option, String worldName) { @Override public void setOption(String option, String value, String worldName) { this.node.set(formatPath(worldName, "options", option), value); + save(); } @Override @@ -149,21 +202,21 @@ public boolean isVirtual() { @Override public void save() { if (isVirtual()) { - this.node.getParent().set(node.getName(), node); + this.config.set(nodePath, node); virtual = false; } try { this.config.save(); } catch (IOException e) { - PermissionsEx.getPermissionManager().getLogger().log(Level.SEVERE, "Error saving data for " + node.getCurrentPath(), e); + PermissionsEx.getPermissionManager().getLogger().log(Level.SEVERE, "Error saving data for " + nodePath, e); } } @Override public void remove() { - node.getParent().set(node.getName(), null); - this.virtual = true; + this.config.set(nodePath, null); + this.save(); } @Override @@ -181,7 +234,7 @@ public Map> getParentsMap() { @Override public List getParents(String worldName) { List parents = this.node.getStringList(formatPath(worldName, parentPath)); - for (Iterator it = parents.iterator(); it.hasNext(); ) { + for (Iterator it = parents.iterator(); it.hasNext();) { final String test = it.next(); if (test == null || test.isEmpty()) { it.remove(); @@ -198,6 +251,7 @@ public List getParents(String worldName) { @Override public void setParents(List parents, String worldName) { this.node.set(formatPath(worldName, parentPath), parents == null ? null : new ArrayList<>(parents)); + save(); } @Override diff --git a/src/main/java/ru/tehkode/permissions/backends/sql/SQLBackend.java b/src/main/java/ru/tehkode/permissions/backends/sql/SQLBackend.java index b09e01c1b..50f6f9ef4 100644 --- a/src/main/java/ru/tehkode/permissions/backends/sql/SQLBackend.java +++ b/src/main/java/ru/tehkode/permissions/backends/sql/SQLBackend.java @@ -48,7 +48,6 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.atomic.AtomicReference; -import java.util.concurrent.locks.ReentrantReadWriteLock; /** * @author code @@ -261,14 +260,14 @@ public String getTableName(String identifier) { @Override public PermissionsUserData getUserData(String name) { - CachingUserData data = new CachingUserData(new SQLData(name, SQLData.Type.USER, this), getExecutor(), new ReentrantReadWriteLock()); + CachingUserData data = new CachingUserData(new SQLData(name, SQLData.Type.USER, this), getExecutor(), new Object()); updateNameCache(userNamesCache, data); return data; } @Override public PermissionsGroupData getGroupData(String name) { - CachingGroupData data = new CachingGroupData(new SQLData(name, SQLData.Type.GROUP, this), getExecutor(), new ReentrantReadWriteLock()); + CachingGroupData data = new CachingGroupData(new SQLData(name, SQLData.Type.GROUP, this), getExecutor(), new Object()); updateNameCache(groupNamesCache, data); return data; }