From c06a0bf8a0553b8887d5f1b45c1c03d343e93ca3 Mon Sep 17 00:00:00 2001 From: Mewel Date: Mon, 17 Feb 2025 15:51:16 +0100 Subject: [PATCH] MCR-3283 Enable PMD rule DoubleCheckedLocking (#2407) * MCR-3283 Start working on PMD rule DoubleCheckedLocking * MCR-3283 Enable PMD rule DoubleCheckedLocking --- .../mycore/access/MCRAccessCacheManager.java | 34 +++------ .../java/org/mycore/common/MCRSession.java | 23 ++++++ .../mycore/datamodel/ifs2/MCRStoreCenter.java | 21 +++++- .../datamodel/ifs2/MCRStoreManager.java | 71 +++++++++++++++++-- .../niofs/ifs2/MCRFileSystemUtils.java | 24 ++----- .../org/mycore/oai/MCROAIDataProvider.java | 36 +++++----- ruleset.xml | 3 - 7 files changed, 139 insertions(+), 73 deletions(-) diff --git a/mycore-base/src/main/java/org/mycore/access/MCRAccessCacheManager.java b/mycore-base/src/main/java/org/mycore/access/MCRAccessCacheManager.java index 6b0b9bc612..5b570f9dc1 100644 --- a/mycore-base/src/main/java/org/mycore/access/MCRAccessCacheManager.java +++ b/mycore-base/src/main/java/org/mycore/access/MCRAccessCacheManager.java @@ -19,7 +19,6 @@ package org.mycore.access; import java.util.List; -import java.util.Optional; import java.util.Set; import java.util.UUID; import java.util.stream.Collectors; @@ -33,38 +32,23 @@ /** * @author Thomas Scheffler (yagee) - * */ class MCRAccessCacheManager { + private static final int CAPACITY = MCRConfiguration2.getOrThrow("MCR.Access.Cache.Size", Integer::valueOf); - private static String key = MCRAccessCacheManager.class.getCanonicalName(); + private static final String SESSION_PERMISSION_KEY = MCRAccessCacheManager.class.getCanonicalName(); + @SuppressWarnings("unchecked") private MCRCache getSessionPermissionCache() { MCRSession session = MCRSessionMgr.getCurrentSession(); - @SuppressWarnings("unchecked") - MCRCache cache = (MCRCache) session.get(key); - if (cache == null) { - synchronized (getCacheCreationLock(session)) { - cache = (MCRCache) session.get(key); - if (cache == null) { - cache = createCache(session); - session.put(key, cache); - } - } - } - return cache; - } - - private static Object getCacheCreationLock(MCRSession session) { - //in a short living scoped session, we should not lock the whole session, but a unique object - return Optional.of(session) - .map(s -> s.get(MCRScopedSession.SCOPED_HINT)) - .orElse(session); + return (MCRCache) session.computeIfAbsent( + SESSION_PERMISSION_KEY, k -> createCache(session)); } + @SuppressWarnings("unchecked") private MCRCache getCacheFromSession(MCRSession session) { - return (MCRCache) session.get(key); + return (MCRCache) session.get(SESSION_PERMISSION_KEY); } private MCRCache createCache(MCRSession session) { @@ -101,7 +85,7 @@ private void removePermissionFromCache(MCRCache pe .stream() .filter(hdl -> hdl.id() != null) .filter(hdl -> ids.contains(hdl.id())) - .collect(Collectors.toList()); + .toList(); handlesToRemove.forEach(permissionCache::remove); } @@ -119,6 +103,6 @@ private record MCRPermissionHandle(String id, String permission) { private MCRPermissionHandle { permission = permission.intern(); } - } + } diff --git a/mycore-base/src/main/java/org/mycore/common/MCRSession.java b/mycore-base/src/main/java/org/mycore/common/MCRSession.java index 731671aa38..e108f14735 100644 --- a/mycore-base/src/main/java/org/mycore/common/MCRSession.java +++ b/mycore-base/src/main/java/org/mycore/common/MCRSession.java @@ -45,6 +45,7 @@ import java.util.concurrent.ThreadFactory; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Function; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -248,6 +249,28 @@ public Object put(Object key, Object value) { return map.put(key, value); } + /** + * Retrieves the value associated with the given key within the session, or computes and stores a new value if + * absent. + *

+ * This method ensures thread-safe lazy initialization by leveraging + * {@link ConcurrentHashMap#computeIfAbsent(Object, Function)}. + *

+ * If the key is not already present in the session, the provided mapping function is used to compute the value, + * which is then stored in the session. + * + * @param key the key whose associated value is to be retrieved or computed + * @param mappingFunction the function to compute a value if the key is absent + * @return the existing or newly computed value associated with the key + * @throws NullPointerException if the key or mapping function is null + */ + public Object computeIfAbsent(Object key, Function mappingFunction) { + if(!map.containsKey(key)) { + mapChanged = true; + } + return map.computeIfAbsent(key, mappingFunction); + } + /** Returns the object that was stored in the session under the given key * */ public Object get(Object key) { return map.get(key); diff --git a/mycore-base/src/main/java/org/mycore/datamodel/ifs2/MCRStoreCenter.java b/mycore-base/src/main/java/org/mycore/datamodel/ifs2/MCRStoreCenter.java index 6857145cc2..5e23aee663 100644 --- a/mycore-base/src/main/java/org/mycore/datamodel/ifs2/MCRStoreCenter.java +++ b/mycore-base/src/main/java/org/mycore/datamodel/ifs2/MCRStoreCenter.java @@ -18,9 +18,10 @@ package org.mycore.datamodel.ifs2; -import java.util.HashMap; import java.util.Map; import java.util.Objects; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Supplier; import java.util.stream.Stream; public final class MCRStoreCenter { @@ -30,7 +31,7 @@ public final class MCRStoreCenter { private static volatile MCRStoreCenter instance; private MCRStoreCenter() { - this.storeHeap = new HashMap<>(); + this.storeHeap = new ConcurrentHashMap<>(); } public static MCRStoreCenter instance() { @@ -52,10 +53,23 @@ public static MCRStoreCenter instance() { */ public void addStore(String id, MCRStore store) throws MCRStoreAlreadyExistsException { if (storeHeap.putIfAbsent(id, store) != null) { - throw new MCRStoreAlreadyExistsException("Could not add store with ID " + id + ", store allready exists"); + throw new MCRStoreAlreadyExistsException("Could not add store with ID " + id + ", store already exists"); } } + /** + * Computes a store if it does not exist yet. + * + * @param id store id + * @param storeSupplier the mapping function to create a store + * @return the original store or the newly computed one + * @param the store + */ + @SuppressWarnings("unchecked") + public T computeStoreIfAbsent(String id, Supplier storeSupplier) { + return (T) storeHeap.computeIfAbsent(id, k -> storeSupplier.get()); + } + /** * Get the MyCoRe Store with the given ID from store center. * @@ -107,4 +121,5 @@ public boolean removeStore(String id) { public void clear() { storeHeap.clear(); } + } diff --git a/mycore-base/src/main/java/org/mycore/datamodel/ifs2/MCRStoreManager.java b/mycore-base/src/main/java/org/mycore/datamodel/ifs2/MCRStoreManager.java index 18619a0457..ec98204481 100644 --- a/mycore-base/src/main/java/org/mycore/datamodel/ifs2/MCRStoreManager.java +++ b/mycore-base/src/main/java/org/mycore/datamodel/ifs2/MCRStoreManager.java @@ -18,33 +18,95 @@ package org.mycore.datamodel.ifs2; +import java.util.function.Supplier; + import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.mycore.common.MCRException; import org.mycore.datamodel.ifs2.MCRStore.MCRStoreConfig; +/** + * Manages the lifecycle of {@link MCRStore} instances, including creation, retrieval, and removal. + *

+ * This class provides methods to create new stores, retrieve existing ones, and remove them from + * the {@link MCRStoreCenter}. It ensures proper initialization and thread-safe store access. + */ public class MCRStoreManager { - private static final Logger LOGGER = LogManager.getLogger(MCRStoreManager.class); + private static final Logger LOGGER = LogManager.getLogger(); + + /** + * Builds a new store instance using the default configuration and adds it to the {@link MCRStoreCenter}. + * + * @param id the unique identifier for the store + * @param storeClass the class of the store to create + * @return the created store instance + * @throws ReflectiveOperationException if the store cannot be instantiated + */ public static T createStore(String id, Class storeClass) throws ReflectiveOperationException { return createStore(new MCRStoreDefaultConfig(id), storeClass); } + /** + * Builds a new store instance with the specified configuration and adds it to the {@link MCRStoreCenter}. + * + * @param config the store configuration + * @param storeClass the class of the store to create + * @return the created store instance + * @throws ReflectiveOperationException if the store cannot be instantiated + */ public static T createStore(MCRStoreConfig config, Class storeClass) throws ReflectiveOperationException { - T store = storeClass.getDeclaredConstructor().newInstance(); - store.init(config); + T store = buildStore(config, storeClass); try { LOGGER.info("Adding instance of {} as MCRStore '{}'.", storeClass::getSimpleName, store::getID); MCRStoreCenter.instance().addStore(store.getID(), store); } catch (Exception e) { - throw new MCRException("Could not create store with ID " + config.getID() + ", store allready exists", e); + throw new MCRException("Could not create store with ID " + config.getID() + ", store already exists", e); } + return store; + } + /** + * Builds a new store instance using the default configuration. + * + * @param id the unique identifier for the store + * @param storeClass the class of the store to create + * @return the built store instance + * @throws ReflectiveOperationException if the store cannot be instantiated + */ + public static T buildStore(String id, Class storeClass) + throws ReflectiveOperationException { + return buildStore(new MCRStoreDefaultConfig(id), storeClass); + } + + /** + * Builds a new store instance with the specified configuration. + * + * @param config the store configuration + * @param storeClass the class of the store to create + * @return the built store instance + * @throws ReflectiveOperationException if the store cannot be instantiated + */ + public static T buildStore(MCRStoreConfig config, Class storeClass) + throws ReflectiveOperationException { + T store = storeClass.getDeclaredConstructor().newInstance(); + store.init(config); return store; } + /** + * Retrieves an existing store by its ID, or creates and registers a new one if absent. + * + * @param id the store ID + * @param storeSupplier the supplier function to create a store if it does not exist + * @return the existing or newly created store instance + */ + public static T computeStoreIfAbsent(String id, Supplier storeSupplier) { + return MCRStoreCenter.instance().computeStoreIfAbsent(id, storeSupplier); + } + /** * Returns the store with the given id * @@ -67,4 +129,5 @@ public static void removeStore(String id) { LOGGER.info("Remove MCRStore '{}'.", id); MCRStoreCenter.instance().removeStore(id); } + } diff --git a/mycore-ifs/src/main/java/org/mycore/datamodel/niofs/ifs2/MCRFileSystemUtils.java b/mycore-ifs/src/main/java/org/mycore/datamodel/niofs/ifs2/MCRFileSystemUtils.java index 4a0346e9a5..9418234b96 100644 --- a/mycore-ifs/src/main/java/org/mycore/datamodel/niofs/ifs2/MCRFileSystemUtils.java +++ b/mycore-ifs/src/main/java/org/mycore/datamodel/niofs/ifs2/MCRFileSystemUtils.java @@ -56,11 +56,10 @@ /** * @author Thomas Scheffler - * */ final class MCRFileSystemUtils { - private static final Logger LOGGER = LogManager.getLogger(MCRFileSystemUtils.class); + private static final Logger LOGGER = LogManager.getLogger(); private static final String DEFAULT_CONFIG_PREFIX = "MCR.IFS.ContentStore.IFS2."; //todo: rename @@ -96,27 +95,16 @@ static MCRFileCollection getFileCollection(String owner) throws IOException { } static MCRFileStore getStore(String base) { - String storeBaseDir = getBaseDir(); - + String baseDir = getBaseDir(); String sid = STORE_ID_PREFIX + base; - storeBaseDir += File.separatorChar + base.replace("_", File.separator); - - MCRFileStore store = MCRStoreManager.getStore(sid); - if (store == null) { - synchronized (MCRStoreManager.class) { - store = MCRStoreManager.getStore(sid); - if (store == null) { - store = createStore(sid, storeBaseDir, base); - } - } - } - return store; + String storeBaseDir = baseDir + File.separatorChar + base.replace("_", File.separator); + return MCRStoreManager.computeStoreIfAbsent(sid, () -> buildStore(sid, storeBaseDir, base)); } - private static MCRFileStore createStore(String sid, String storeBaseDir, String base) { + private static MCRFileStore buildStore(String sid, String storeBaseDir, String base) { try { configureStore(sid, storeBaseDir, base); - return MCRStoreManager.createStore(sid, MCRFileStore.class); + return MCRStoreManager.buildStore(sid, MCRFileStore.class); } catch (Exception ex) { String msg = "Could not create IFS2 file store with ID " + sid; throw new MCRConfigurationException(msg, ex); diff --git a/mycore-oai/src/main/java/org/mycore/oai/MCROAIDataProvider.java b/mycore-oai/src/main/java/org/mycore/oai/MCROAIDataProvider.java index a43f529623..cd9ef8e5c4 100644 --- a/mycore-oai/src/main/java/org/mycore/oai/MCROAIDataProvider.java +++ b/mycore-oai/src/main/java/org/mycore/oai/MCROAIDataProvider.java @@ -26,6 +26,7 @@ import java.util.List; import java.util.Map; import java.util.ServiceLoader; +import java.util.concurrent.ConcurrentHashMap; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -73,7 +74,7 @@ public class MCROAIDataProvider extends MCRServlet { private static final OAIXMLOutputProcessor OAI_XML_OUTPUT_PROCESSOR = new OAIXMLOutputProcessor(); static { - ADAPTER_MAP = new HashMap<>(); + ADAPTER_MAP = new ConcurrentHashMap<>(); } private String myBaseURL; @@ -105,16 +106,16 @@ protected void doGetPost(MCRServletJob job) throws Exception { // handle request OAIResponse oaiResponse = oaiProvider.handleRequest(oaiRequest); // build response - Element xmlRespone = oaiResponse.toXML(); + Element xmlResponse = oaiResponse.toXML(); if (!(adapter instanceof MCROAIAdapter mcrAdapter) || mcrAdapter.moveNamespaceDeclarationsToRoot()) { - moveNamespacesUp(xmlRespone); + moveNamespacesUp(xmlResponse); } // fire job.getResponse().setContentType("text/xml; charset=UTF-8"); XMLOutputter xout = new XMLOutputter(Format.getPrettyFormat(), OAI_XML_OUTPUT_PROCESSOR); - xout.output(addXSLStyle(new Document(xmlRespone)), job.getResponse().getOutputStream()); + xout.output(addXSLStyle(new Document(xmlResponse)), job.getResponse().getOutputStream()); } /** @@ -165,28 +166,23 @@ private Document addXSLStyle(Document doc) { private OAIAdapter getOAIAdapter() { String oaiAdapterKey = getServletName(); - MCROAIAdapter oaiAdapter = ADAPTER_MAP.get(oaiAdapterKey); - if (oaiAdapter == null) { - synchronized (this) { - // double check because of synchronize block - oaiAdapter = ADAPTER_MAP.get(oaiAdapterKey); - if (oaiAdapter == null) { - String adapter = MCROAIAdapter.PREFIX + oaiAdapterKey + ".Adapter"; - oaiAdapter = MCRConfiguration2.getInstanceOf(MCROAIAdapter.class, adapter) - .orElseGet(() -> MCRConfiguration2.getInstanceOfOrThrow( - MCROAIAdapter.class, MCROAIAdapter.PREFIX + "DefaultAdapter")); - oaiAdapter.init(this.myBaseURL, oaiAdapterKey); - ADAPTER_MAP.put(oaiAdapterKey, oaiAdapter); - } - } - } + return ADAPTER_MAP.computeIfAbsent(oaiAdapterKey, k -> createOAIAdapter(oaiAdapterKey)); + } + + private MCROAIAdapter createOAIAdapter(String oaiAdapterKey) { + MCROAIAdapter oaiAdapter; + String adapter = MCROAIAdapter.PREFIX + oaiAdapterKey + ".Adapter"; + oaiAdapter = MCRConfiguration2.getInstanceOf(MCROAIAdapter.class, adapter) + .orElseGet(() -> MCRConfiguration2.getInstanceOfOrThrow( + MCROAIAdapter.class, MCROAIAdapter.PREFIX + "DefaultAdapter")); + oaiAdapter.init(this.myBaseURL, oaiAdapterKey); return oaiAdapter; } /** * Moves all namespace declarations in the children of target to the target. * - * @param target the namespace are bundled here + * @param target the namespace is bundled here */ private void moveNamespacesUp(Element target) { Map existingNamespaces = getNamespaceMap(target); diff --git a/ruleset.xml b/ruleset.xml index dced0b22d0..ae9e2ca319 100644 --- a/ruleset.xml +++ b/ruleset.xml @@ -313,10 +313,7 @@ --> - -