Author: markt
Date: Fri Sep 14 08:41:02 2018
New Revision: 1840901
URL: http://svn.apache.org/viewvc?rev=1840901&view=rev
Log:
Review thread-safety
Document locking strategy
Fix a few issues:
- Iterators could throw ConcurrentModificationException
- Syncs on open/save didn't lock roles Map
Update with a view to supporting runtime reloading (BZ 58590)
Modified:
tomcat/trunk/java/org/apache/catalina/users/MemoryUserDatabase.java
Modified: tomcat/trunk/java/org/apache/catalina/users/MemoryUserDatabase.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/users/MemoryUserDatabase.java?rev=1840901&r1=1840900&r2=1840901&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/users/MemoryUserDatabase.java
(original)
+++ tomcat/trunk/java/org/apache/catalina/users/MemoryUserDatabase.java Fri Sep
14 08:41:02 2018
@@ -22,8 +22,12 @@ import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStreamWriter;
import java.io.PrintWriter;
-import java.util.HashMap;
+import java.util.ArrayList;
import java.util.Iterator;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
import org.apache.catalina.Globals;
import org.apache.catalina.Group;
@@ -42,10 +46,34 @@ import org.xml.sax.Attributes;
* Concrete implementation of {@link UserDatabase} that loads all defined
users,
* groups, and roles into an in-memory data structure, and uses a specified XML
* file for its persistent storage.
+ * <p>
+ * This class is thread-safe.
+ * <p>
+ * This class does not enforce what, in an RDBMS, would be called referential
+ * integrity. Concurrent modifications may result in inconsistent data such as
+ * a User retaining a reference to a Role that has been removed from the
+ * database.
*
* @author Craig R. McClanahan
* @since 4.1
*/
+/*
+ * Implementation notes:
+ *
+ * Any operation that acts on a single element of the database (e.g. operations
+ * that create, read, update or delete a user, role or group) must first obtain
+ * the read lock. Operations that return iterators for users, roles or groups
+ * also fall into this category.
+ *
+ * Iterators must always be created from copies of the data to prevent possible
+ * corruption of the iterator due to the remove of all elements from the
+ * underlying Map that would occur during a subsequent re-loading of the
+ * database.
+ *
+ * Any operation that acts on multiple elements and expects the database to
+ * remain consistent during the operation (e.g. saving or loading the database)
+ * must first obtain the write lock.
+ */
public class MemoryUserDatabase implements UserDatabase {
private static final Log log = LogFactory.getLog(MemoryUserDatabase.class);
@@ -76,7 +104,7 @@ public class MemoryUserDatabase implemen
/**
* The set of {@link Group}s defined in this database, keyed by group name.
*/
- protected final HashMap<String, Group> groups = new HashMap<>();
+ protected final Map<String, Group> groups = new ConcurrentHashMap<>();
/**
* The unique global identifier of this user database.
@@ -109,12 +137,16 @@ public class MemoryUserDatabase implemen
/**
* The set of {@link Role}s defined in this database, keyed by role name.
*/
- protected final HashMap<String, Role> roles = new HashMap<>();
+ protected final Map<String, Role> roles = new ConcurrentHashMap<>();
/**
* The set of {@link User}s defined in this database, keyed by user name.
*/
- protected final HashMap<String, User> users = new HashMap<>();
+ protected final Map<String, User> users = new ConcurrentHashMap<>();
+
+ private final ReentrantReadWriteLock dbLock = new ReentrantReadWriteLock();
+ private final Lock readLock = dbLock.readLock();
+ private final Lock writeLock = dbLock.writeLock();
// ------------------------------------------------------------- Properties
@@ -124,8 +156,11 @@ public class MemoryUserDatabase implemen
*/
@Override
public Iterator<Group> getGroups() {
- synchronized (groups) {
- return groups.values().iterator();
+ readLock.lock();
+ try {
+ return new ArrayList<>(groups.values()).iterator();
+ } finally {
+ readLock.unlock();
}
}
@@ -182,8 +217,11 @@ public class MemoryUserDatabase implemen
*/
@Override
public Iterator<Role> getRoles() {
- synchronized (roles) {
- return roles.values().iterator();
+ readLock.lock();
+ try {
+ return new ArrayList<>(roles.values()).iterator();
+ } finally {
+ readLock.unlock();
}
}
@@ -193,8 +231,11 @@ public class MemoryUserDatabase implemen
*/
@Override
public Iterator<User> getUsers() {
- synchronized (users) {
- return users.values().iterator();
+ readLock.lock();
+ try {
+ return new ArrayList<>(users.values()).iterator();
+ } finally {
+ readLock.unlock();
}
}
@@ -209,13 +250,14 @@ public class MemoryUserDatabase implemen
@Override
public void close() throws Exception {
- save();
-
- synchronized (groups) {
- synchronized (users) {
- users.clear();
- groups.clear();
- }
+ writeLock.lock();
+ try {
+ save();
+ users.clear();
+ groups.clear();
+ roles.clear();
+ } finally {
+ writeLock.unlock();
}
}
@@ -235,8 +277,11 @@ public class MemoryUserDatabase implemen
}
MemoryGroup group = new MemoryGroup(this, groupname, description);
- synchronized (groups) {
+ readLock.lock();
+ try {
groups.put(group.getGroupname(), group);
+ } finally {
+ readLock.unlock();
}
return group;
}
@@ -257,8 +302,11 @@ public class MemoryUserDatabase implemen
}
MemoryRole role = new MemoryRole(this, rolename, description);
- synchronized (roles) {
+ readLock.lock();
+ try {
roles.put(role.getRolename(), role);
+ } finally {
+ readLock.unlock();
}
return role;
}
@@ -281,8 +329,11 @@ public class MemoryUserDatabase implemen
}
MemoryUser user = new MemoryUser(this, username, password, fullName);
- synchronized (users) {
+ readLock.unlock();
+ try {
users.put(user.getUsername(), user);
+ } finally {
+ readLock.unlock();
}
return user;
}
@@ -296,8 +347,11 @@ public class MemoryUserDatabase implemen
*/
@Override
public Group findGroup(String groupname) {
- synchronized (groups) {
+ readLock.lock();
+ try {
return groups.get(groupname);
+ } finally {
+ readLock.unlock();
}
}
@@ -310,8 +364,11 @@ public class MemoryUserDatabase implemen
*/
@Override
public Role findRole(String rolename) {
- synchronized (roles) {
+ readLock.lock();
+ try {
return roles.get(rolename);
+ } finally {
+ readLock.unlock();
}
}
@@ -324,8 +381,11 @@ public class MemoryUserDatabase implemen
*/
@Override
public User findUser(String username) {
- synchronized (users) {
+ readLock.lock();
+ try {
return users.get(username);
+ } finally {
+ readLock.unlock();
}
}
@@ -338,37 +398,37 @@ public class MemoryUserDatabase implemen
@Override
public void open() throws Exception {
- synchronized (groups) {
- synchronized (users) {
-
- // Erase any previous groups and users
- users.clear();
- groups.clear();
- roles.clear();
-
- String pathName = getPathname();
- try (InputStream is =
ConfigFileLoader.getInputStream(getPathname())) {
- // Construct a digester to read the XML input file
- Digester digester = new Digester();
- try {
- digester.setFeature(
-
"http://apache.org/xml/features/allow-java-encodings";, true);
- } catch (Exception e) {
-
log.warn(sm.getString("memoryUserDatabase.xmlFeatureEncoding"), e);
- }
- digester.addFactoryCreate("tomcat-users/group",
- new MemoryGroupCreationFactory(this), true);
- digester.addFactoryCreate("tomcat-users/role",
- new MemoryRoleCreationFactory(this), true);
- digester.addFactoryCreate("tomcat-users/user",
- new MemoryUserCreationFactory(this), true);
-
- // Parse the XML input to load this database
- digester.parse(is);
- } catch (IOException ioe) {
- log.error(sm.getString("memoryUserDatabase.fileNotFound",
pathName));
+ writeLock.lock();
+ try {
+ // Erase any previous groups and users
+ users.clear();
+ groups.clear();
+ roles.clear();
+
+ String pathName = getPathname();
+ try (InputStream is =
ConfigFileLoader.getInputStream(getPathname())) {
+ // Construct a digester to read the XML input file
+ Digester digester = new Digester();
+ try {
+ digester.setFeature(
+
"http://apache.org/xml/features/allow-java-encodings";, true);
+ } catch (Exception e) {
+
log.warn(sm.getString("memoryUserDatabase.xmlFeatureEncoding"), e);
}
+ digester.addFactoryCreate("tomcat-users/group",
+ new MemoryGroupCreationFactory(this), true);
+ digester.addFactoryCreate("tomcat-users/role",
+ new MemoryRoleCreationFactory(this), true);
+ digester.addFactoryCreate("tomcat-users/user",
+ new MemoryUserCreationFactory(this), true);
+
+ // Parse the XML input to load this database
+ digester.parse(is);
+ } catch (IOException ioe) {
+ log.error(sm.getString("memoryUserDatabase.fileNotFound",
pathName));
}
+ } finally {
+ writeLock.unlock();
}
}
@@ -380,14 +440,16 @@ public class MemoryUserDatabase implemen
*/
@Override
public void removeGroup(Group group) {
-
- synchronized (groups) {
+ readLock.lock();
+ try {
Iterator<User> users = getUsers();
while (users.hasNext()) {
User user = users.next();
user.removeGroup(group);
}
groups.remove(group.getGroupname());
+ } finally {
+ readLock.unlock();
}
}
@@ -399,8 +461,8 @@ public class MemoryUserDatabase implemen
*/
@Override
public void removeRole(Role role) {
-
- synchronized (roles) {
+ readLock.lock();
+ try {
Iterator<Group> groups = getGroups();
while (groups.hasNext()) {
Group group = groups.next();
@@ -412,6 +474,8 @@ public class MemoryUserDatabase implemen
user.removeRole(role);
}
roles.remove(role.getRolename());
+ } finally {
+ readLock.unlock();
}
}
@@ -423,8 +487,11 @@ public class MemoryUserDatabase implemen
*/
@Override
public void removeUser(User user) {
- synchronized (users) {
+ readLock.lock();
+ try {
users.remove(user.getUsername());
+ } finally {
+ readLock.unlock();
}
}
@@ -484,22 +551,27 @@ public class MemoryUserDatabase implemen
writer.println("xsi:schemaLocation=\"http://tomcat.apache.org/xml
tomcat-users.xsd\"");
writer.println(" version=\"1.0\">");
- // Print entries for each defined role, group, and user
- Iterator<?> values = null;
- values = getRoles();
- while (values.hasNext()) {
- writer.print(" ");
- writer.println(values.next());
- }
- values = getGroups();
- while (values.hasNext()) {
- writer.print(" ");
- writer.println(values.next());
- }
- values = getUsers();
- while (values.hasNext()) {
- writer.print(" ");
- writer.println(((MemoryUser) values.next()).toXml());
+ writeLock.lock();
+ try {
+ // Print entries for each defined role, group, and user
+ Iterator<?> values = null;
+ values = getRoles();
+ while (values.hasNext()) {
+ writer.print(" ");
+ writer.println(values.next());
+ }
+ values = getGroups();
+ while (values.hasNext()) {
+ writer.print(" ");
+ writer.println(values.next());
+ }
+ values = getUsers();
+ while (values.hasNext()) {
+ writer.print(" ");
+ writer.println(((MemoryUser) values.next()).toXml());
+ }
+ } finally {
+ writeLock.unlock();
}
// Print the file epilog
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
