(struts) 07/14: WW-5539 Deduplicate the no-mapping path in computeMappingIfAbsent

[lu...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

lukaszlenart pushed a commit to branch WW-5539
in repository https://gitbox.apache.org/repos/asf/struts.git

commit 1bc7b33d168f9d15271a91a9eb7b3698ea2ef52b
Author: Lukasz Lenart <lu...@apache.org>
AuthorDate: Tue Jul 21 17:18:15 2026 +0200

    WW-5539 Deduplicate the no-mapping path in computeMappingIfAbsent
    
    ConcurrentHashMap.computeIfAbsent stores nothing when the mapping function
    returns null, so every concurrent caller re-ran the builder for a class
    with no conversion mapping - the common case for an ordinary action, and
    the exact thundering herd this method exists to prevent.
    
    Negative results now store a sentinel in the same map, so the builder runs
    once per class either way. getMapping and containsNoMapping translate the
    sentinel, preserving their existing contracts.
---
 .../conversion/StrutsTypeConverterHolder.java      | 35 ++++++++++------------
 .../conversion/StrutsTypeConverterHolderTest.java  | 27 +++++++++++++++++
 2 files changed, 43 insertions(+), 19 deletions(-)

diff --git a/core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java b/core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java
index 247f2badc..4b53b1dda 100644
--- a/core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java
+++ b/core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java
@@ -22,6 +22,7 @@ import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
@@ -65,9 +66,12 @@ public class StrutsTypeConverterHolder implements TypeConverterHolder {
     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 Set<Class> noMapping = ConcurrentHashMap.newKeySet(); // action
+    private static final Map<String, Object> NO_MAPPING = Collections.unmodifiableMap(new HashMap<>());
 
     /**
      * Record classes that doesn't have conversion mapping defined.
@@ -103,7 +107,8 @@ public class StrutsTypeConverterHolder implements TypeConverterHolder {
     @Override
     @Deprecated
     public Map<String, Object> getMapping(Class clazz) {
-        return mappings.get(clazz);
+        Map<String, Object> mapping = mappings.get(clazz);
+        return mapping == NO_MAPPING ? null : mapping;
     }
 
     @Override
@@ -115,28 +120,20 @@ public class StrutsTypeConverterHolder implements TypeConverterHolder {
     @Override
     @Deprecated
     public boolean containsNoMapping(Class clazz) {
-        return noMapping.contains(clazz);
+        return mappings.get(clazz) == NO_MAPPING;
     }
 
     @Override
-    public Map<String, Object> computeMappingIfAbsent(Class clazz, Function<Class, Map<String, Object>> builder) {
-        if (noMapping.contains(clazz)) {
-            return Collections.emptyMap();
-        }
-        Map<String, Object> mapping = mappings.computeIfAbsent(clazz, c -> {
-            Map<String, Object> built = builder.apply(c);
-            return (built == null || built.isEmpty()) ? null : built;
-        });
-        if (mapping == null) {
-            noMapping.add(clazz);
-            return Collections.emptyMap();
-        }
-        return mapping;
+    public void addNoMapping(Class clazz) {
+        mappings.put(clazz, NO_MAPPING);
     }
 
     @Override
-    public void addNoMapping(Class clazz) {
-        noMapping.add(clazz);
+    public Map<String, Object> computeMappingIfAbsent(Class clazz, Function<Class, Map<String, Object>> builder) {
+        return mappings.computeIfAbsent(clazz, c -> {
+            Map<String, Object> built = builder.apply(c);
+            return (built == null || built.isEmpty()) ? NO_MAPPING : built;
+        });
     }
 
     @Override
diff --git a/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java b/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java
index efb7f0fc1..34dc18ad0 100644
--- a/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java
+++ b/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java
@@ -222,4 +222,31 @@ public class StrutsTypeConverterHolderTest extends XWorkTestCase {
 
         assertThat(builds.get()).as("mapping must be built exactly once").isEqualTo(1);
     }
+
+    public void testComputeMappingIfAbsentBuildsOnceUnderConcurrencyForUnmappedClass() throws Exception {
+        StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder();
+        AtomicInteger builds = new AtomicInteger();
+        ExecutorService pool = Executors.newFixedThreadPool(THREADS);
+        CountDownLatch start = new CountDownLatch(1);
+        List<Future<Map<String, Object>>> futures = new ArrayList<>();
+
+        for (int t = 0; t < THREADS; t++) {
+            futures.add(pool.submit(() -> {
+                start.await();
+                return holder.computeMappingIfAbsent(String.class, clazz -> {
+                    builds.incrementAndGet();
+                    return Collections.emptyMap();
+                });
+            }));
+        }
+
+        start.countDown();
+        for (Future<Map<String, Object>> future : futures) {
+            assertThat(future.get(60, TimeUnit.SECONDS)).isEmpty();
+        }
+        pool.shutdown();
+
+        assertThat(builds.get()).as("unmapped class must be built exactly once").isEqualTo(1);
+        assertThat(holder.containsNoMapping(String.class)).isTrue();
+    }
 }