MCR-3884 fix thread-safety of static caches

mycore-base/src/main/java/org/mycore/datamodel/classifications2/impl/MCRCategoryDAOImpl.java

@@ -32,11 +32,12 @@
 import java.util.AbstractMap;
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.SortedSet;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicLong;
 import java.util.function.BiConsumer;
 import java.util.function.Consumer;
 import java.util.function.Function;
@@ -73,13 +74,13 @@ public class MCRCategoryDAOImpl implements MCRCategoryDAO {
 
     private static final int LEFT_START_VALUE = 0;
 
-    private static long lastModified = System.currentTimeMillis();
+    private static final AtomicLong LAST_MODIFIED = new AtomicLong(System.currentTimeMillis());
 
     private static final Logger LOGGER = LogManager.getLogger();
 
     private static final String NAMED_QUERY_NAMESPACE = "MCRCategory.";
 
-    private static final Map<String, Long> LAST_MODIFIED_MAP = new HashMap<>();
+    private static final Map<String, Long> LAST_MODIFIED_MAP = new ConcurrentHashMap<>();
 
     @Override
     public MCRCategory addCategory(MCRCategoryID parentID, MCRCategory category) {
@@ -575,11 +576,19 @@ public void repairLeftRightValue(String classID) {
 
     @Override
     public long getLastModified() {
-        return lastModified;
+        return LAST_MODIFIED.get();
     }
 
     private static void updateTimeStamp() {
-        lastModified = System.currentTimeMillis();
+        LAST_MODIFIED.accumulateAndGet(System.currentTimeMillis(), MCRCategoryDAOImpl::nextTimestamp);
+    }
+
+    /**
+     * Returns the current time, but at least <code>last + 1</code>. Caches like <code>MCRRoleManager</code> compare
+     * timestamps for equality and would miss a second change within the same millisecond otherwise.
+     */
+    private static long nextTimestamp(long last, long now) {
+        return Math.max(last + 1, now);
     }
 
     private static MCRCategoryImpl buildCategoryFromPrefetchedList(List<MCRCategoryDTO> list, MCRCategoryID returnID) {
@@ -697,8 +706,8 @@ private static void updateLeftRightValue(EntityManager entityManager, String cla
      * Method updates the last modified timestamp, for the given root id.
      *
      */
-    protected synchronized void updateLastModified(String root) {
-        LAST_MODIFIED_MAP.put(root, System.currentTimeMillis());
+    protected void updateLastModified(String root) {
+        LAST_MODIFIED_MAP.merge(root, System.currentTimeMillis(), MCRCategoryDAOImpl::nextTimestamp);
     }
 
     /**
@@ -708,11 +717,7 @@ protected synchronized void updateLastModified(String root) {
      */
     @Override
     public long getLastModified(String root) {
-        Long long1 = LAST_MODIFIED_MAP.get(root);
-        if (long1 != null) {
-            return long1;
-        }
-        return -1;
+        return LAST_MODIFIED_MAP.getOrDefault(root, -1L);
     }
 
     @SuppressWarnings("unchecked")

mycore-base/src/main/java/org/mycore/frontend/MCRLayoutUtilities.java

@@ -29,10 +29,10 @@
 import java.net.URLEncoder;
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.Executor;
 import java.util.concurrent.Executors;
 import java.util.concurrent.TimeUnit;
@@ -109,8 +109,6 @@ public class MCRLayoutUtilities {
     private static final boolean ACCESS_CONTROLL_ON = MCRConfiguration2
         .getOrThrow("MCR.Website.ReadAccessVerification", Boolean::parseBoolean);
 
-    private static Map<String, Element> itemStore = new HashMap<>();
-
     private static final LoadingCache<String, DocumentHolder> NAV_DOCUMENT_CACHE = CacheBuilder.newBuilder()
         .refreshAfterWrite(STANDARD_CACHE_SECONDS, TimeUnit.SECONDS).build(new CacheLoader<>() {
 
@@ -119,12 +117,7 @@ public class MCRLayoutUtilities {
             @Override
             public DocumentHolder load(String key) throws Exception {
                 URL url = MCRResourceHelper.getWebResourceUrl(key);
-                try {
-                    return new DocumentHolder(url);
-                } finally {
-                    itemStore.clear();
-                }
-
+                return new DocumentHolder(url);
             }
 
             @Override
@@ -284,11 +277,15 @@ public static boolean getAccess(String webpageID, String permission, int strateg
      * Returns a Element presentation of an item[@href=$webpageID]
      */
     private static Element getItem(String webpageID) {
-        Element item = itemStore.get(webpageID);
+        // items are cached per document, so a reloaded navigation.xml never returns stale items
+        DocumentHolder navigation = NAV_DOCUMENT_CACHE.getUnchecked(NAV_RESOURCE);
+        Element item = webpageID == null ? null : navigation.items.get(webpageID);
         if (item == null) {
             XPathExpression<Element> xpath = XPATH_FACTORY.compile("//.[@href='" + webpageID + "']", Filters.element());
-            item = xpath.evaluateFirst(getNavi());
-            itemStore.put(webpageID, item);
+            item = xpath.evaluateFirst(navigation.parsedDocument);
+            if (item != null && webpageID != null) {
+                navigation.items.put(webpageID, item);
+            }
         }
         return item;
     }
@@ -475,6 +472,9 @@ private static class DocumentHolder {
 
         long lastModified;
 
+        /** items of {@link #parsedDocument}, key is the webpage ID (item/@href) */
+        final Map<String, Element> items = new ConcurrentHashMap<>();
+
         DocumentHolder(URL url) throws JDOMException, IOException {
             docURL = url;
             parseDocument();

mycore-base/src/test/java/org/mycore/datamodel/classifications2/impl/MCRCategoryDAOImplTest.java

@@ -600,6 +600,23 @@ public void setLabel() {
         assertEquals(description, rootNode.getLabel(lang).get().getDescription(), "Label does not match.");
     }
 
+    @Test
+    public void lastModifiedIncreasesOnEveryChange() {
+        addWorldClassification();
+        String rootID = category.getId().getRootID();
+        long lastModified = DAO.getLastModified();
+        long rootLastModified = DAO.getLastModified(rootID);
+        // changes within the same millisecond must be detectable, too
+        for (int i = 0; i < 10; i++) {
+            DAO.setLabel(category.getId(), new MCRLabel("ju", "JUnit-Test " + i, null));
+            assertTrue(DAO.getLastModified() > lastModified, "Last modified timestamp did not increase.");
+            assertTrue(DAO.getLastModified(rootID) > rootLastModified,
+                "Last modified timestamp of " + rootID + " did not increase.");
+            lastModified = DAO.getLastModified();
+            rootLastModified = DAO.getLastModified(rootID);
+        }
+    }
+
     @Test
     public void setLabels() {
         addWorldClassification();

mycore-user2/src/main/java/org/mycore/user2/MCRRoleManager.java

@@ -21,7 +21,7 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
-import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.SortedSet;
@@ -49,39 +49,37 @@ public class MCRRoleManager {
 
     private static final Logger LOGGER = LogManager.getLogger();
 
-    /** Map of defined roles, key is the unique role name */
-    private static Map<String, MCRRole> rolesByName = new HashMap<>();
-
-    /** List of all defined roles */
-    private static List<MCRRole> rolesList = new ArrayList<>();
-
     private static final MCRCategoryDAO DAO = MCRCategoryDAOFactory.obtainInstance();
 
-    private static long lastLoaded;
+    /** defined system roles, replaced as a whole if the classification changes */
+    private static volatile SystemRoles systemRoles = new SystemRoles(-1, Map.of(), List.of());
 
     private static final MCRCategLinkService CATEG_LINK_SERVICE = MCRCategLinkServiceFactory.obtainInstance();
 
     static {
         loadSystemRoles();
     }
 
-    private static void loadSystemRoles() {
-        if (lastLoaded == DAO.getLastModified() || !DAO.exist(MCRUser2Constants.ROLE_CLASSID)) {
-            return;
+    private static SystemRoles loadSystemRoles() {
+        SystemRoles current = systemRoles;
+        long lastModified = DAO.getLastModified();
+        if (current.lastLoaded() == lastModified || !DAO.exist(MCRUser2Constants.ROLE_CLASSID)) {
+            return current;
         }
-        lastLoaded = DAO.getLastModified();
         int onlyNonHierarchicalRoles = 1;
         MCRCategory roleCategory = DAO.getCategory(MCRUser2Constants.ROLE_CLASSID, onlyNonHierarchicalRoles);
-        rolesByName.clear();
-        rolesList.clear();
+        Map<String, MCRRole> rolesByName = new LinkedHashMap<>();
         if (roleCategory != null) {
             for (MCRCategory child : roleCategory.getChildren()) {
                 String name = child.getId().getId();
-                MCRRole role = new MCRRole(name, child.getLabels());
-                rolesByName.put(name, role);
-                rolesList.add(role);
+                rolesByName.put(name, new MCRRole(name, child.getLabels()));
             }
         }
+        // never modify the current instance, as other threads may read it concurrently
+        SystemRoles loaded = new SystemRoles(lastModified, Map.copyOf(rolesByName),
+            List.copyOf(rolesByName.values()));
+        systemRoles = loaded;
+        return loaded;
     }
 
     /**
@@ -91,8 +89,7 @@ private static void loadSystemRoles() {
      * @return the role with the given role name, or null.
      */
     public static MCRRole getRole(String name) {
-        loadSystemRoles();
-        MCRRole mcrRole = rolesByName.get(name);
+        MCRRole mcrRole = loadSystemRoles().rolesByName().get(name);
         if (mcrRole == null) {
             return getExternalRole(name);
         }
@@ -124,7 +121,7 @@ public static MCRRole getExternalRole(String name) {
      * @return array each element either MCRRole instance, or null
      */
     public static MCRRole[] getRoles(String... names) {
-        loadSystemRoles();
+        Map<String, MCRRole> rolesByName = loadSystemRoles().rolesByName();
         MCRRole[] roles = new MCRRole[names.length];
         for (int i = 0; i < names.length; i++) {
             roles[i] = rolesByName.get(names[i]);
@@ -142,7 +139,6 @@ public static MCRRole[] getRoles(String... names) {
      * @return collection each element is MCRRole instance.
      */
     public static Collection<MCRRole> getRoles(Collection<String> names) {
-        loadSystemRoles();
         String[] namesArr = names.toArray(String[]::new);
         MCRRole[] roles = getRoles(namesArr);
         return Arrays.asList(roles);
@@ -151,10 +147,10 @@ public static Collection<MCRRole> getRoles(Collection<String> names) {
     /**
      * Returns a list of all defined roles
      *
-     * @return a list of all defined roles
+     * @return an unmodifiable list of all defined roles
      */
     public static List<MCRRole> listSystemRoles() {
-        return rolesList;
+        return loadSystemRoles().rolesList();
     }
 
     /**
@@ -261,4 +257,15 @@ private static MCRCategoryID getCategoryID(MCRRole role) {
         }
         return MCRCategoryID.ofString(role.getName());
     }
+
+    /**
+     * Immutable snapshot of the system roles.
+     *
+     * @param lastLoaded last modified timestamp of the classifications when the roles were loaded
+     * @param rolesByName defined roles, key is the unique role name
+     * @param rolesList all defined roles
+     */
+    private record SystemRoles(long lastLoaded, Map<String, MCRRole> rolesByName, List<MCRRole> rolesList) {
+    }
+
 }

mycore-user2/src/test/java/org/mycore/user2/MCRRoleManagerTest.java

@@ -0,0 +1,64 @@
+/*
+ * This file is part of ***  M y C o R e  ***
+ * See https://www.mycore.de/ for details.
+ *
+ * MyCoRe is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * MyCoRe is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with MyCoRe.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+package org.mycore.user2;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.util.List;
+import java.util.Set;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mycore.datamodel.classifications2.MCRLabel;
+import org.mycore.test.MCRJPAExtension;
+import org.mycore.test.MyCoReTest;
+
+@MyCoReTest
+@ExtendWith({ MCRJPAExtension.class, MCRUserExtension.class })
+public class MCRRoleManagerTest {
+
+    @Test
+    public void listSystemRoles() {
+        List<MCRRole> roles = MCRRoleManager.listSystemRoles();
+        assertEquals(List.of("admin", "editor"), roles.stream().map(MCRRole::getName).toList());
+        assertThrows(UnsupportedOperationException.class, roles::clear,
+            "List of system roles should not be modifiable.");
+    }
+
+    @Test
+    public void addAndDeleteRole() {
+        List<MCRRole> roles = MCRRoleManager.listSystemRoles();
+        assertNull(MCRRoleManager.getRole("submitter"));
+
+        MCRRoleManager.addRole(new MCRRole("submitter", Set.of(new MCRLabel("en", "Submitter", null))));
+        assertNotNull(MCRRoleManager.getRole("submitter"), "Added role should be available immediately.");
+        assertEquals(List.of("admin", "editor", "submitter"),
+            MCRRoleManager.listSystemRoles().stream().map(MCRRole::getName).toList());
+        assertEquals(2, roles.size(), "Previously returned list should not change.");
+
+        MCRRoleManager.deleteRole("submitter");
+        assertNull(MCRRoleManager.getRole("submitter"), "Deleted role should not be available anymore.");
+        assertEquals(List.of("admin", "editor"),
+            MCRRoleManager.listSystemRoles().stream().map(MCRRole::getName).toList());
+    }
+
+}