WW-5539 Concurrency performance enhancements

core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java

@@ -18,23 +18,32 @@
  */
 package org.apache.struts2.conversion;
 
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Function;
 
 /**
  * Default implementation of {@link TypeConverterHolder}
  */
 public class StrutsTypeConverterHolder implements TypeConverterHolder {
 
+    private static final Logger LOG = LogManager.getLogger(StrutsTypeConverterHolder.class);
+
     /**
      * Record class and its type converter mapping.
      * <pre>
      * - String - classname as String
      * - TypeConverter - instance of TypeConverter
      * </pre>
      */
-    private final HashMap<String, TypeConverter> defaultMappings = new HashMap<>();  // non-action (eg. returned value)
+    private final Map<String, TypeConverter> defaultMappings = new ConcurrentHashMap<>();  // non-action (eg. returned value)
 
     /**
      * Target class conversion Mappings.
@@ -55,25 +64,47 @@
      *                    Element_property=foo.bar.MyObject
      * </pre>
      */
-    private final HashMap<Class, Map<String, Object>> mappings = new HashMap<>(); // action
+    private final Map<Class, Map<String, Object>> mappings = new ConcurrentHashMap<>(); // action
 
     /**
-     * Unavailable target class conversion mappings, serves as a simple cache.
+     * Marker stored in {@link #mappings} for classes known to have no conversion mapping, so that
+     * negative results are cached in the same atomic operation as positive ones. Deliberately a
+     * distinct instance rather than {@link Collections#emptyMap()}, whose shared singleton could
+     * collide with an empty mapping supplied by a caller.
      */
-    private final HashSet<Class> noMapping = new HashSet<>(); // action
+    private static final Map<String, Object> NO_MAPPING = Collections.unmodifiableMap(new HashMap<>());
 
     /**
      * Record classes that doesn't have conversion mapping defined.
      * <pre>
      * - String -&gt; classname as String
      * </pre>
+     *
+     * @deprecated since 7.3.0, unused - superseded by internal concurrent storage. Retained only
+     * for binary compatibility with subclasses compiled against earlier versions, and will be
+     * removed in a future release.
+     */
+    @Deprecated(since = "7.3.0", forRemoval = true)
+    protected HashSet<String> unknownMappings = new HashSet<>();
+
+    /**
+     * Actual storage for classes with no registered converter. Concurrent, so that lock-free
+     * readers in {@code XWorkConverter.lookup} cannot race writers.
      */
-    protected HashSet<String> unknownMappings = new HashSet<>();     // non-action (eg. returned value)
+    private final Set<String> unknownMappingsInternal = ConcurrentHashMap.newKeySet();
 
     @Override
     public void addDefaultMapping(String className, TypeConverter typeConverter) {
+        if (typeConverter == null) {
+            LOG.warn("Ignoring null TypeConverter registered for class [{}]", className);
+            return;
+        }
+        // Order is load-bearing: registering the converter before clearing the unknown flag means a
+        // concurrent XWorkConverter.lookup can never observe (unknown=false, default=false) for this
+        // class - a state that would otherwise send it down the lookupSuper() path and let it
+        // overwrite the more specific converter being registered here with a broader one.
         defaultMappings.put(className, typeConverter);
-        unknownMappings.remove(className);
+        unknownMappingsInternal.remove(className);
     }
 
     @Override
@@ -86,34 +117,84 @@
         return defaultMappings.get(className);
     }
 
+    /**
+     * Returns {@code null} if {@code clazz} has been flagged as having no mapping via
+     * {@link #addNoMapping(Class)}, even if a real mapping was previously stored for it.
+     *
+     * @deprecated since 7.3.0, use {@link #computeMappingIfAbsent(Class, Function)} instead.
+     */
+    // Implementing the deprecated interface primitives is mandatory until they are removed.
+    @SuppressWarnings("removal")
     @Override
+    @Deprecated(since = "7.3.0", forRemoval = true)
     public Map<String, Object> getMapping(Class clazz) {
-        return mappings.get(clazz);
+        Map<String, Object> mapping = mappings.get(clazz);
+        return mapping == NO_MAPPING ? null : mapping;
     }
 
+    /**
+     * @deprecated since 7.3.0, use {@link #computeMappingIfAbsent(Class, Function)} instead.
+     */
+    @SuppressWarnings("removal")
     @Override
+    @Deprecated(since = "7.3.0", forRemoval = true)
     public void addMapping(Class clazz, Map<String, Object> mapping) {
         mappings.put(clazz, mapping);
     }
 
+    /**
+     * @deprecated since 7.3.0, use {@link #computeMappingIfAbsent(Class, Function)} instead.
+     */
+    @SuppressWarnings("removal")
     @Override
+    @Deprecated(since = "7.3.0", forRemoval = true)
     public boolean containsNoMapping(Class clazz) {
-        return noMapping.contains(clazz);
+        return mappings.get(clazz) == NO_MAPPING;
     }
 
+    /**
+     * Stores the {@link #NO_MAPPING} sentinel for the given class, replacing any mapping previously
+     * cached for it. This matches the pre-7.3.0 effective behaviour, back when a separate no-mapping
+     * collection was consulted independently of the mappings map: the only in-tree caller,
+     * {@code XWorkConverter.getConverter}, checked the no-mapping flag first and short-circuited
+     * before the cached mapping was ever read, so flagging a class as no-mapping made it behave as
+     * though it had no mapping, real cached mapping or not. {@link Map#putIfAbsent} is deliberately
+     * not used here: it would leave a stale mapping being served for a class whose conversion build
+     * subsequently failed, which is a genuine behaviour change rather than a faithful port.
+     */
     @Override
     public void addNoMapping(Class clazz) {
-        noMapping.add(clazz);
+        mappings.put(clazz, NO_MAPPING);
+    }
+
+    @Override
+    public Map<String, Object> computeMappingIfAbsent(Class clazz, Function<Class, Map<String, Object>> builder) {
+        // Deliberately not implemented with mappings.computeIfAbsent(...): that would run the builder
+        // while holding the ConcurrentHashMap's internal bin lock. The builder reaches
+        // ObjectFactory.buildConverter(...), which instantiates (and, under SpringObjectFactory,
+        // autowires) an arbitrary user-supplied TypeConverter - constructors, @PostConstruct,
+        // afterPropertiesSet. Running that under a bin lock risks IllegalStateException("Recursive
+        // update") or a self-deadlock if any of it re-enters conversion. Instead the builder runs
+        // outside any lock, at the cost of allowing it to run more than once under first-access
+        // contention; putIfAbsent ensures every caller still converges on the same cached instance.
+        Map<String, Object> existing = mappings.get(clazz);
+        if (existing != null) {
+            return existing;
+        }
+        Map<String, Object> built = builder.apply(clazz);
+        Map<String, Object> value = (built == null || built.isEmpty()) ? NO_MAPPING : built;
+        Map<String, Object> previous = mappings.putIfAbsent(clazz, value);
+        return previous != null ? previous : value;
     }
 
     @Override
     public boolean containsUnknownMapping(String className) {
-        return unknownMappings.contains(className);
+        return unknownMappingsInternal.contains(className);
     }
 
     @Override
     public void addUnknownMapping(String className) {
-        unknownMappings.add(className);
+        unknownMappingsInternal.add(className);
     }
 
 }

core/src/main/java/org/apache/struts2/conversion/TypeConverterHolder.java

@@ -18,7 +18,9 @@
  */
 package org.apache.struts2.conversion;
 
+import java.util.Collections;
 import java.util.Map;
+import java.util.function.Function;
 
 /**
  * Holds all mappings related to {@link TypeConverter}s
@@ -52,29 +54,44 @@
     /**
      * Target class conversion Mappings.
      *
+     * <p>Returns {@code null} if the class has been flagged as having no mapping via
+     * {@link #addNoMapping(Class)}, even if a real mapping was previously stored for it with
+     * {@link #addMapping(Class, Map)}.</p>
+     *
      * @param clazz class to convert to/from
-     * @return {@link TypeConverter} for given class
+     * @return the property-converter mapping for the given class, or {@code null} if none
+     * @deprecated since 7.3.0, use {@link #computeMappingIfAbsent(Class, Function)}, which resolves
+     * and caches the mapping in one call instead of requiring a check-then-act at the call site.
      */
+    @Deprecated(since = "7.3.0", forRemoval = true)
     Map<String, Object> getMapping(Class clazz);
 
     /**
      * Assign mapping of converters for given class
      *
      * @param clazz   class to convert to/from
      * @param mapping property converters
+     * @deprecated since 7.3.0, use {@link #computeMappingIfAbsent(Class, Function)} which stores
+     * the built mapping itself.
      */
+    @Deprecated(since = "7.3.0", forRemoval = true)
     void addMapping(Class clazz, Map<String, Object> mapping);
 
     /**
      * Check if there is no mapping for given class to convert
      *
      * @param clazz class to convert to/from
      * @return true if mapping couldn't be found
+     * @deprecated since 7.3.0, use {@link #computeMappingIfAbsent(Class, Function)} which returns
+     * an empty map for classes known to have no mapping.
      */
+    @Deprecated(since = "7.3.0", forRemoval = true)
     boolean containsNoMapping(Class clazz);
 
     /**
-     * Adds no mapping flag for give class
+     * Adds no mapping flag for give class. Flagging a class as having no mapping may replace
+     * any mapping previously cached for it; callers should treat this flag as authoritative
+     * over a previously cached mapping.
      *
      * @param clazz class to register missing converter
      */
@@ -97,4 +114,42 @@
      */
     void addUnknownMapping(String className);
 
+    /**
+     * Returns the property-converter mapping for the given class, building and caching it on first
+     * use. Never returns {@code null}: a class known to have no mapping yields an empty map.
+     *
+     * <p>If the builder returns {@code null} or an empty map, the class is recorded in the negative
+     * cache so the builder is not invoked for it again.</p>
+     *
+     * <p>Implementations only guarantee that all callers converge on the same cached mapping
+     * instance for a given class - not that the builder runs at most once. Under concurrent first
+     * access, the builder may be invoked more than once (each on a different thread, for the same
+     * class); only one of the resulting mappings is retained and returned to every caller. The
+     * builder must therefore be idempotent and free of side effects on the holder itself. The
+     * default implementation is a non-atomic check-then-act using the deprecated primitives,
+     * preserving pre-7.3.0 behaviour for third-party holders that do not override it.</p>
+     *
+     * @param clazz   class to convert to/from
+     * @param builder builds the property-converter mapping for the class when it is not yet cached
+     * @return the mapping for the class, or an empty map if it has none
+     * @since 7.3.0
+     */
+    @SuppressWarnings("deprecation")
+    default Map<String, Object> computeMappingIfAbsent(Class clazz, Function<Class, Map<String, Object>> builder) {
+        if (containsNoMapping(clazz)) {
+            return Collections.emptyMap();
+        }
+        Map<String, Object> mapping = getMapping(clazz);
+        if (mapping != null) {
+            return mapping;
+        }
+        mapping = builder.apply(clazz);
+        if (mapping == null || mapping.isEmpty()) {
+            addNoMapping(clazz);
+            return Collections.emptyMap();
+        }
+        addMapping(clazz, mapping);
+        return mapping;
+    }
+
 }

core/src/main/java/org/apache/struts2/conversion/impl/XWorkConverter.java

@@ -412,34 +412,43 @@ public TypeConverter lookup(Class clazz) {
     }
 
     protected Object getConverter(Class clazz, String property) {
-        LOG.debug("Retrieving convert for class [{}] and property [{}]", clazz, property);
+        LOG.debug("Retrieving converter for class [{}] and property [{}]", clazz, property);
 
-        synchronized (clazz) {
-            if ((property != null) && !converterHolder.containsNoMapping(clazz)) {
-                try {
-                    Map<String, Object> mapping = converterHolder.getMapping(clazz);
-
-                    if (mapping == null) {
-                        mapping = buildConverterMapping(clazz);
-                    } else {
-                        mapping = conditionalReload(clazz, mapping);
-                    }
+        if (property == null) {
+            return null;
+        }
+        try {
+            Map<String, Object> mapping = converterHolder.computeMappingIfAbsent(clazz, this::buildConverterMappingUnchecked);
+            if (!mapping.isEmpty()) {
+                mapping = conditionalReload(clazz, mapping);
+            }
 
-                    Object converter = mapping.get(property);
-                    if (converter == null && LOG.isDebugEnabled()) {
-                        LOG.debug("Converter is null for property [{}]. Mapping size [{}]:", property, mapping.size());
-                        for (Map.Entry<String, Object> entry : mapping.entrySet()) {
-                            LOG.debug("{}:{}", entry.getKey(), entry.getValue());
-                        }
-                    }
-                    return converter;
-                } catch (Throwable t) {
-                    LOG.debug("Got exception trying to resolve convert for class [{}] and property [{}]", clazz, property, t);
-                    converterHolder.addNoMapping(clazz);
+            Object converter = mapping.get(property);
+            if (converter == null && LOG.isDebugEnabled()) {
+                LOG.debug("Converter is null for property [{}]. Mapping size [{}]:", property, mapping.size());
+                for (Map.Entry<String, Object> entry : mapping.entrySet()) {
+                    LOG.debug("{}:{}", entry.getKey(), entry.getValue());
                 }
             }
+            return converter;
+        } catch (Throwable t) {
+            LOG.debug("Got exception trying to resolve converter for class [{}] and property [{}]", clazz, property, t);
+            converterHolder.addNoMapping(clazz);
+            return null;
+        }
+    }
+
+    /**
+     * Adapts {@link #buildConverterMapping(Class)} to {@link java.util.function.Function} by
+     * rethrowing its checked exception unchecked. The caller's {@code catch (Throwable)} still
+     * negative-caches the class, so behaviour is unchanged.
+     */
+    private Map<String, Object> buildConverterMappingUnchecked(Class clazz) {
+        try {
+            return buildConverterMapping(clazz);
+        } catch (Exception e) {
+            throw new IllegalStateException("Could not build converter mapping for " + clazz, e);
         }
-        return null;
     }
 
     protected void handleConversionException(Map<String, Object> context, String property, Object value, Object object, Class toClass) {
@@ -463,11 +472,11 @@ protected void handleConversionException(Map<String, Object> context, String pro
         }
     }
 
-    public synchronized void registerConverter(String className, TypeConverter converter) {
+    public void registerConverter(String className, TypeConverter converter) {
         converterHolder.addDefaultMapping(className, converter);
     }
 
-    public synchronized void registerConverterNotFound(String className) {
+    public void registerConverterNotFound(String className) {
         converterHolder.addUnknownMapping(className);
     }
 
@@ -547,6 +556,8 @@ protected void addConverterMapping(Map<String, Object> mapping, Class clazz) {
      * @param clazz the class to look for converter mappings for
      * @return the converter mappings
      * @throws Exception in case of any errors
+     * @since 7.3.0 this method no longer stores the built mapping in the {@link TypeConverterHolder};
+     * storage is owned by {@link TypeConverterHolder#computeMappingIfAbsent(Class, java.util.function.Function)}.
      */
     protected Map<String, Object> buildConverterMapping(Class clazz) throws Exception {
         Map<String, Object> mapping = new HashMap<>();
@@ -568,22 +579,24 @@ protected Map<String, Object> buildConverterMapping(Class clazz) throws Exceptio
             curClazz = curClazz.getSuperclass();
         }
 
-        if (!mapping.isEmpty()) {
-            converterHolder.addMapping(clazz, mapping);
-        } else {
-            converterHolder.addNoMapping(clazz);
-        }
-
         return mapping;
     }
 
+    @SuppressWarnings({"deprecation", "removal"})
     private Map<String, Object> conditionalReload(Class clazz, Map<String, Object> oldValues) throws Exception {
         Map<String, Object> mapping = oldValues;
 
         if (reloadingConfigs) {
             URL fileUrl = ClassLoaderUtil.getResource(buildConverterFilename(clazz), clazz);
             if (fileManager.fileNeedsReloading(fileUrl)) {
                 mapping = buildConverterMapping(clazz);
+                if (mapping.isEmpty()) {
+                    converterHolder.addNoMapping(clazz);
+                } else {
+                    // addMapping is deprecated but remains the correct primitive here:
+                    // computeMappingIfAbsent cannot express an unconditional overwrite.
+                    converterHolder.addMapping(clazz, mapping);
+                }
             }
         }