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 <[email protected]> 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(); + } }
