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 3013c3bd47..4b3d5ca382 100644
--- a/core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java
+++ b/core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java
@@ -18,15 +18,24 @@
*/
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.
*
@@ -34,7 +43,7 @@ public class StrutsTypeConverterHolder implements TypeConverterHolder {
* - TypeConverter - instance of TypeConverter
*
*/
- private final HashMap defaultMappings = new HashMap<>(); // non-action (eg. returned value)
+ private final Map defaultMappings = new ConcurrentHashMap<>(); // non-action (eg. returned value)
/**
* Target class conversion Mappings.
@@ -55,25 +64,47 @@ public class StrutsTypeConverterHolder implements TypeConverterHolder {
* Element_property=foo.bar.MyObject
*
*/
- private final HashMap> mappings = new HashMap<>(); // action
+ private final Map> 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 noMapping = new HashSet<>(); // action
+ private static final Map NO_MAPPING = Collections.unmodifiableMap(new HashMap<>());
/**
* Record classes that doesn't have conversion mapping defined.
*
* - String -> classname as String
*
+ *
+ * @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 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 unknownMappings = new HashSet<>(); // non-action (eg. returned value)
+ private final Set 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 @@ public TypeConverter getDefaultMapping(String className) {
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 getMapping(Class clazz) {
- return mappings.get(clazz);
+ Map 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 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 computeMappingIfAbsent(Class clazz, Function> 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 existing = mappings.get(clazz);
+ if (existing != null) {
+ return existing;
+ }
+ Map built = builder.apply(clazz);
+ Map value = (built == null || built.isEmpty()) ? NO_MAPPING : built;
+ Map 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);
}
}
diff --git a/core/src/main/java/org/apache/struts2/conversion/TypeConverterHolder.java b/core/src/main/java/org/apache/struts2/conversion/TypeConverterHolder.java
index d20324719d..8fc1c507ae 100644
--- a/core/src/main/java/org/apache/struts2/conversion/TypeConverterHolder.java
+++ b/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,9 +54,16 @@ public interface TypeConverterHolder {
/**
* Target class conversion Mappings.
*
+ *
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)}.
+ *
* @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 getMapping(Class clazz);
/**
@@ -62,7 +71,10 @@ public interface TypeConverterHolder {
*
* @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 mapping);
/**
@@ -70,11 +82,16 @@ public interface TypeConverterHolder {
*
* @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 @@ public interface TypeConverterHolder {
*/
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.
+ *
+ *
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.
+ *
+ *
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.
+ *
+ * @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 computeMappingIfAbsent(Class clazz, Function> builder) {
+ if (containsNoMapping(clazz)) {
+ return Collections.emptyMap();
+ }
+ Map 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;
+ }
+
}
diff --git a/core/src/main/java/org/apache/struts2/conversion/impl/XWorkConverter.java b/core/src/main/java/org/apache/struts2/conversion/impl/XWorkConverter.java
index 89d3ecceb4..5579a15d3e 100644
--- a/core/src/main/java/org/apache/struts2/conversion/impl/XWorkConverter.java
+++ b/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 mapping = converterHolder.getMapping(clazz);
-
- if (mapping == null) {
- mapping = buildConverterMapping(clazz);
- } else {
- mapping = conditionalReload(clazz, mapping);
- }
+ if (property == null) {
+ return null;
+ }
+ try {
+ Map 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 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 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 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 context, String property, Object value, Object object, Class toClass) {
@@ -463,11 +472,11 @@ protected void handleConversionException(Map 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 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 buildConverterMapping(Class clazz) throws Exception {
Map mapping = new HashMap<>();
@@ -568,15 +579,10 @@ protected Map 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 conditionalReload(Class clazz, Map oldValues) throws Exception {
Map mapping = oldValues;
@@ -584,6 +590,13 @@ private Map conditionalReload(Class clazz, Map o
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);
+ }
}
}
diff --git a/core/src/main/java/org/apache/struts2/validator/DefaultActionValidatorManager.java b/core/src/main/java/org/apache/struts2/validator/DefaultActionValidatorManager.java
index 2f2b758b79..7da4bccf86 100644
--- a/core/src/main/java/org/apache/struts2/validator/DefaultActionValidatorManager.java
+++ b/core/src/main/java/org/apache/struts2/validator/DefaultActionValidatorManager.java
@@ -35,13 +35,11 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
-import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.TreeSet;
-
-import static java.util.Collections.synchronizedMap;
+import java.util.concurrent.ConcurrentHashMap;
/**
*
@@ -67,8 +65,8 @@ public class DefaultActionValidatorManager implements ActionValidatorManager {
*/
protected static final String VALIDATION_CONFIG_SUFFIX = "-validation.xml";
- protected final Map> validatorCache = synchronizedMap(new HashMap<>());
- protected final Map> validatorFileCache = synchronizedMap(new HashMap<>());
+ protected final Map> validatorCache = new ConcurrentHashMap<>();
+ protected final Map> validatorFileCache = new ConcurrentHashMap<>();
private static final Logger LOG = LogManager.getLogger(DefaultActionValidatorManager.class);
protected ValidatorFactory validatorFactory;
@@ -137,17 +135,19 @@ protected Validator getValidatorFromValidatorConfig(ValidatorConfig config, Valu
}
@Override
- public synchronized List getValidators(Class clazz, String context, String method) {
+ public List getValidators(Class clazz, String context, String method) {
String validatorKey = buildValidatorKey(clazz, context);
- if (!validatorCache.containsKey(validatorKey)) {
- validatorCache.put(validatorKey, buildValidatorConfigs(clazz, context, false, null));
+ List configs = validatorCache.get(validatorKey);
+ if (configs == null) {
+ configs = validatorCache.computeIfAbsent(validatorKey,
+ key -> buildValidatorConfigs(clazz, context, false, null));
} else if (reloadingConfigs) {
- validatorCache.put(validatorKey, buildValidatorConfigs(clazz, context, true, null));
+ configs = buildValidatorConfigs(clazz, context, true, null);
+ validatorCache.put(validatorKey, configs);
}
ValueStack stack = ActionContext.getContext().getValueStack();
- List configs = validatorCache.get(validatorKey);
List validators = new ArrayList<>();
for (ValidatorConfig config : configs) {
if (method == null || method.equals(config.getParams().get("methodName"))) {
@@ -158,7 +158,7 @@ public synchronized List getValidators(Class clazz, String context
}
@Override
- public synchronized List getValidators(Class clazz, String context) {
+ public List getValidators(Class clazz, String context) {
return getValidators(clazz, context, null);
}
@@ -283,7 +283,7 @@ protected List buildValidatorConfigs(Class clazz, String con
if (checked == null) {
checked = new TreeSet<>();
} else if (checked.contains(clazz.getName())) {
- return validatorConfigs;
+ return Collections.emptyList();
}
if (clazz.isInterface()) {
@@ -314,7 +314,7 @@ protected List buildValidatorConfigs(Class clazz, String con
}
checked.add(clazz.getName());
- return validatorConfigs;
+ return Collections.unmodifiableList(validatorConfigs);
}
protected List buildAliasValidatorConfigs(Class aClass, String context, boolean checkFile) {
@@ -328,22 +328,35 @@ protected List buildClassValidatorConfigs(Class aClass, bool
}
protected List loadFile(String fileName, Class clazz, boolean checkFile) {
- List retList = Collections.emptyList();
-
URL fileUrl = ClassLoaderUtil.getResource(fileName, clazz);
- if ((checkFile && fileManager.fileNeedsReloading(fileUrl)) || !validatorFileCache.containsKey(fileName)) {
- try (InputStream is = fileManager.loadFile(fileUrl)) {
- if (is != null) {
- retList = new ArrayList<>(validatorFileParser.parseActionValidatorConfigs(validatorFactory, is, fileName));
- }
- } catch (IOException e) {
- LOG.error("Caught exception while closing file {}", fileName, e);
- }
+ if (checkFile && fileManager.fileNeedsReloading(fileUrl)) {
+ List reloaded = parseValidatorConfigs(fileUrl, fileName);
+ validatorFileCache.put(fileName, reloaded);
+ return reloaded;
+ }
- validatorFileCache.put(fileName, retList);
- } else {
- retList = validatorFileCache.get(fileName);
+ return validatorFileCache.computeIfAbsent(fileName, key -> parseValidatorConfigs(fileUrl, fileName));
+ }
+
+ /**
+ * Parses the validator configs from the given file, returning an unmodifiable list. Returns an
+ * empty list when the file does not exist or cannot be read.
+ *
+ * @param fileUrl URL of the validation config file, may be null
+ * @param fileName name of the validation config file, used for logging and parser context
+ * @return an unmodifiable list of validator configs, never null
+ */
+ protected List parseValidatorConfigs(URL fileUrl, String fileName) {
+ List retList = Collections.emptyList();
+
+ try (InputStream is = fileManager.loadFile(fileUrl)) {
+ if (is != null) {
+ retList = Collections.unmodifiableList(
+ new ArrayList<>(validatorFileParser.parseActionValidatorConfigs(validatorFactory, is, fileName)));
+ }
+ } catch (IOException e) {
+ LOG.error("Caught exception while closing file {}", fileName, e);
}
return retList;
diff --git a/core/src/main/java/org/apache/struts2/validator/DefaultValidatorFactory.java b/core/src/main/java/org/apache/struts2/validator/DefaultValidatorFactory.java
index 4e8a233088..d26971bc2d 100644
--- a/core/src/main/java/org/apache/struts2/validator/DefaultValidatorFactory.java
+++ b/core/src/main/java/org/apache/struts2/validator/DefaultValidatorFactory.java
@@ -36,10 +36,10 @@
import java.net.URL;
import java.util.ArrayList;
import java.util.Arrays;
-import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
import java.util.zip.ZipEntry;
import java.util.zip.ZipInputStream;
@@ -52,7 +52,7 @@
*/
public class DefaultValidatorFactory implements ValidatorFactory, Initializable {
- protected Map validators = new HashMap<>();
+ protected Map validators = new ConcurrentHashMap<>();
private static final Logger LOG = LogManager.getLogger(DefaultValidatorFactory.class);
protected ObjectFactory objectFactory;
protected ValidatorFileParser validatorFileParser;
diff --git a/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java b/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java
new file mode 100644
index 0000000000..df28e2dfa9
--- /dev/null
+++ b/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java
@@ -0,0 +1,442 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.struts2.conversion;
+
+import org.apache.struts2.XWorkTestCase;
+
+import java.lang.reflect.Field;
+import java.util.AbstractSet;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class StrutsTypeConverterHolderTest extends XWorkTestCase {
+
+ private static final int THREADS = 16;
+ private static final int PER_THREAD = 200;
+
+ private static TypeConverter stubConverter() {
+ return (context, target, member, propertyName, value, toType) -> null;
+ }
+
+ public void testConcurrentDefaultMappingRegistrationLosesNothing() throws Exception {
+ StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder();
+ ExecutorService pool = Executors.newFixedThreadPool(THREADS);
+ CountDownLatch start = new CountDownLatch(1);
+ List> futures = new ArrayList<>();
+
+ for (int t = 0; t < THREADS; t++) {
+ final int threadId = t;
+ futures.add(pool.submit(() -> {
+ start.await();
+ for (int i = 0; i < PER_THREAD; i++) {
+ String className = "stub.Class" + threadId + "_" + i;
+ holder.addDefaultMapping(className, stubConverter());
+ // interleave reads with writes to provoke the race
+ holder.containsDefaultMapping("stub.Class0_0");
+ holder.getDefaultMapping(className);
+ }
+ return null;
+ }));
+ }
+
+ start.countDown();
+ for (Future future : futures) {
+ future.get(60, TimeUnit.SECONDS);
+ }
+ pool.shutdown();
+
+ for (int t = 0; t < THREADS; t++) {
+ for (int i = 0; i < PER_THREAD; i++) {
+ String className = "stub.Class" + t + "_" + i;
+ assertThat(holder.getDefaultMapping(className))
+ .as("lost registration for %s", className)
+ .isNotNull();
+ }
+ }
+ }
+
+ public void testConcurrentUnknownMappingRegistrationLosesNothing() throws Exception {
+ StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder();
+ ExecutorService pool = Executors.newFixedThreadPool(THREADS);
+ CountDownLatch start = new CountDownLatch(1);
+ List> futures = new ArrayList<>();
+
+ for (int t = 0; t < THREADS; t++) {
+ final int threadId = t;
+ futures.add(pool.submit(() -> {
+ start.await();
+ for (int i = 0; i < PER_THREAD; i++) {
+ holder.addUnknownMapping("stub.Unknown" + threadId + "_" + i);
+ holder.containsUnknownMapping("stub.Unknown0_0");
+ }
+ return null;
+ }));
+ }
+
+ start.countDown();
+ for (Future future : futures) {
+ future.get(60, TimeUnit.SECONDS);
+ }
+ pool.shutdown();
+
+ for (int t = 0; t < THREADS; t++) {
+ for (int i = 0; i < PER_THREAD; i++) {
+ String className = "stub.Unknown" + t + "_" + i;
+ assertThat(holder.containsUnknownMapping(className))
+ .as("lost unknown mapping for %s", className)
+ .isTrue();
+ }
+ }
+ }
+
+ public void testAddDefaultMappingIgnoresNullConverter() {
+ StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder();
+
+ holder.addDefaultMapping("stub.NullConverter", null);
+
+ assertThat(holder.containsDefaultMapping("stub.NullConverter")).isFalse();
+ assertThat(holder.getDefaultMapping("stub.NullConverter")).isNull();
+ }
+
+ public void testAddDefaultMappingClearsUnknownMapping() {
+ StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder();
+
+ holder.addUnknownMapping("stub.Later");
+ assertThat(holder.containsUnknownMapping("stub.Later")).isTrue();
+
+ holder.addDefaultMapping("stub.Later", stubConverter());
+
+ assertThat(holder.containsUnknownMapping("stub.Later")).isFalse();
+ assertThat(holder.getDefaultMapping("stub.Later")).isNotNull();
+ }
+
+ /**
+ * A {@link Map} that pauses the call to {@code put} for one specific key immediately after the
+ * underlying write has taken effect, so a test can deterministically observe state exactly
+ * between two writes without depending on lucky thread scheduling.
+ */
+ private static final class PausingAfterPutMap extends ConcurrentHashMap {
+ private final String watchedKey;
+ private final CountDownLatch writeStarted;
+ private final CountDownLatch release;
+
+ PausingAfterPutMap(String watchedKey, CountDownLatch writeStarted, CountDownLatch release) {
+ this.watchedKey = watchedKey;
+ this.writeStarted = writeStarted;
+ this.release = release;
+ }
+
+ @Override
+ public TypeConverter put(String key, TypeConverter value) {
+ TypeConverter result = super.put(key, value);
+ pauseIfWatched(key, watchedKey, writeStarted, release);
+ return result;
+ }
+ }
+
+ /**
+ * A {@link Set} that pauses the call to {@code remove} for one specific key immediately after
+ * the underlying write has taken effect, mirroring {@link PausingAfterPutMap} for the unknown-
+ * mappings collection.
+ */
+ private static final class PausingAfterRemoveSet extends AbstractSet {
+ private final Set delegate = ConcurrentHashMap.newKeySet();
+ private final String watchedKey;
+ private final CountDownLatch writeStarted;
+ private final CountDownLatch release;
+
+ PausingAfterRemoveSet(String watchedKey, CountDownLatch writeStarted, CountDownLatch release) {
+ this.watchedKey = watchedKey;
+ this.writeStarted = writeStarted;
+ this.release = release;
+ }
+
+ @Override
+ public boolean add(String s) {
+ return delegate.add(s);
+ }
+
+ @Override
+ public boolean remove(Object o) {
+ boolean result = delegate.remove(o);
+ pauseIfWatched(o, watchedKey, writeStarted, release);
+ return result;
+ }
+
+ @Override
+ public boolean contains(Object o) {
+ return delegate.contains(o);
+ }
+
+ @Override
+ public Iterator iterator() {
+ return delegate.iterator();
+ }
+
+ @Override
+ public int size() {
+ return delegate.size();
+ }
+ }
+
+ /**
+ * Signals {@code writeStarted} and blocks on {@code release} the first time it is called for
+ * {@code watchedKey}. Safe to wire into both backing collections with the same latch pair:
+ * whichever write executes first pauses here for the test to inspect state; the other write's
+ * call is a no-op (both latches are already at zero by the time it runs).
+ */
+ private static void pauseIfWatched(Object key, String watchedKey, CountDownLatch writeStarted, CountDownLatch release) {
+ if (!watchedKey.equals(key)) {
+ return;
+ }
+ writeStarted.countDown();
+ try {
+ if (!release.await(10, TimeUnit.SECONDS)) {
+ throw new IllegalStateException("release latch was never counted down");
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new IllegalStateException(e);
+ }
+ }
+
+ private static void injectField(Object target, String fieldName, Object value) throws Exception {
+ Field field = StrutsTypeConverterHolder.class.getDeclaredField(fieldName);
+ field.setAccessible(true);
+ field.set(target, value);
+ }
+
+ /**
+ * Pins the write ordering inside {@code addDefaultMapping}: registering the converter must
+ * become visible before the unknown-mapping flag is cleared. If the order were inverted, a
+ * concurrent reader could observe {@code (containsUnknownMapping == false && containsDefaultMapping
+ * == false)} for a class that started out flagged unknown - a state that sends
+ * {@code XWorkConverter.lookup} down the {@code lookupSuper()} path, letting it overwrite the more
+ * specific converter this method is in the middle of registering with a broader one.
+ *
+ *
The window between the two writes is a couple of instructions wide, so rather than relying
+ * on a lucky interleaving, this replaces the holder's two backing collections with variants that
+ * deterministically pause immediately after whichever one is written first, letting the test
+ * inspect the exact intermediate state {@code addDefaultMapping} produces.
+ */
+ public void testAddDefaultMappingNeverExposesForbiddenIntermediateStateToReaders() throws Exception {
+ String className = "stub.OrderingProbe";
+ StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder();
+ holder.addUnknownMapping(className);
+ assertThat(holder.containsUnknownMapping(className)).isTrue();
+ assertThat(holder.containsDefaultMapping(className)).isFalse();
+
+ CountDownLatch writeStarted = new CountDownLatch(1);
+ CountDownLatch release = new CountDownLatch(1);
+ injectField(holder, "defaultMappings", new PausingAfterPutMap(className, writeStarted, release));
+ injectField(holder, "unknownMappingsInternal", new PausingAfterRemoveSet(className, writeStarted, release));
+
+ ExecutorService pool = Executors.newSingleThreadExecutor();
+ try {
+ Future writer = pool.submit(() -> holder.addDefaultMapping(className, stubConverter()));
+
+ assertThat(writeStarted.await(10, TimeUnit.SECONDS))
+ .as("addDefaultMapping's first write did not happen in time")
+ .isTrue();
+
+ // Snapshot exactly between the two writes, whichever order they run in.
+ boolean unknownFlagged = holder.containsUnknownMapping(className);
+ boolean defaultFlagged = holder.containsDefaultMapping(className);
+
+ release.countDown();
+ writer.get(10, TimeUnit.SECONDS);
+
+ assertThat(unknownFlagged || defaultFlagged)
+ .as("a concurrent reader observed (unknown=false, default=false) mid-registration for "
+ + "[%s] - addDefaultMapping must register the converter before clearing the "
+ + "unknown flag", className)
+ .isTrue();
+ } finally {
+ pool.shutdown();
+ }
+
+ assertThat(holder.containsUnknownMapping(className)).isFalse();
+ assertThat(holder.getDefaultMapping(className)).isNotNull();
+ }
+
+ public void testComputeMappingIfAbsentBuildsOnceAndCaches() {
+ StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder();
+ AtomicInteger builds = new AtomicInteger();
+
+ Map first = holder.computeMappingIfAbsent(String.class, clazz -> {
+ builds.incrementAndGet();
+ Map built = new HashMap<>();
+ built.put("someProperty", "someConverter");
+ return built;
+ });
+ Map second = holder.computeMappingIfAbsent(String.class, clazz -> {
+ builds.incrementAndGet();
+ return new HashMap<>();
+ });
+
+ assertThat(builds.get()).isEqualTo(1);
+ assertThat(first).containsEntry("someProperty", "someConverter");
+ assertThat(second).isSameAs(first);
+ }
+
+ public void testComputeMappingIfAbsentNegativeCachesEmptyResult() {
+ StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder();
+ AtomicInteger builds = new AtomicInteger();
+
+ Map first = holder.computeMappingIfAbsent(String.class, clazz -> {
+ builds.incrementAndGet();
+ return Collections.emptyMap();
+ });
+ Map second = holder.computeMappingIfAbsent(String.class, clazz -> {
+ builds.incrementAndGet();
+ return Collections.emptyMap();
+ });
+
+ assertThat(first).isEmpty();
+ assertThat(second).isEmpty();
+ assertThat(builds.get()).as("empty result must be negative cached").isEqualTo(1);
+ assertThat(holder.containsNoMapping(String.class)).isTrue();
+ assertThat(holder.getMapping(String.class)).isNull();
+ }
+
+ /**
+ * Covers the branches of {@code getMapping} and {@code containsNoMapping} that are not the
+ * {@link StrutsTypeConverterHolder#NO_MAPPING} sentinel: a class with a real cached mapping
+ * must get that mapping back (not {@code null}), and must not be reported as having no mapping.
+ */
+ public void testGetMappingAndContainsNoMappingReflectRealMapping() {
+ StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder();
+ Map real = new HashMap<>();
+ real.put("someProperty", "someConverter");
+
+ holder.addMapping(String.class, real);
+
+ assertThat(holder.getMapping(String.class)).isSameAs(real);
+ assertThat(holder.containsNoMapping(String.class)).isFalse();
+ }
+
+ public void testAddNoMappingOverridesPreviouslyCachedMapping() {
+ StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder();
+ Map real = new HashMap<>();
+ real.put("someProperty", "someConverter");
+ holder.addMapping(String.class, real);
+
+ holder.addNoMapping(String.class);
+
+ assertThat(holder.containsNoMapping(String.class)).isTrue();
+ assertThat(holder.getMapping(String.class)).isNull();
+ assertThat(holder.computeMappingIfAbsent(String.class, clazz -> {
+ throw new AssertionError("builder must not run when no mapping is set");
+ })).isNotNull().isEmpty();
+ }
+
+ public void testComputeMappingIfAbsentNegativeCachesNullResult() {
+ StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder();
+
+ Map result = holder.computeMappingIfAbsent(String.class, clazz -> null);
+
+ assertThat(result).isNotNull().isEmpty();
+ assertThat(holder.containsNoMapping(String.class)).isTrue();
+ }
+
+ public void testComputeMappingIfAbsentShortCircuitsOnKnownNoMapping() {
+ StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder();
+ holder.addNoMapping(String.class);
+
+ Map result = holder.computeMappingIfAbsent(String.class, clazz -> {
+ throw new AssertionError("builder must not run for a negative-cached class");
+ });
+
+ assertThat(result).isNotNull().isEmpty();
+ }
+
+ /**
+ * {@code computeMappingIfAbsent} deliberately does not run the builder inside a lock (see the
+ * comment on the production method), so under concurrent first access the builder may run more
+ * than once. What must still hold - and is the property callers actually depend on - is that
+ * every caller converges on the same cached mapping instance, with the correct content.
+ */
+ public void testComputeMappingIfAbsentConvergesOnSameInstanceUnderConcurrency() throws Exception {
+ StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder();
+ ExecutorService pool = Executors.newFixedThreadPool(THREADS);
+ CountDownLatch start = new CountDownLatch(1);
+ List>> futures = new ArrayList<>();
+
+ for (int t = 0; t < THREADS; t++) {
+ futures.add(pool.submit(() -> {
+ start.await();
+ return holder.computeMappingIfAbsent(String.class, clazz -> {
+ Map built = new HashMap<>();
+ built.put("someProperty", "someConverter");
+ return built;
+ });
+ }));
+ }
+
+ start.countDown();
+ Map expected = futures.get(0).get(60, TimeUnit.SECONDS);
+ for (Future> future : futures) {
+ assertThat(future.get(60, TimeUnit.SECONDS)).isSameAs(expected);
+ }
+ pool.shutdown();
+
+ assertThat(expected).containsEntry("someProperty", "someConverter");
+ }
+
+ /**
+ * Same property as above, for the negative-cache path: every caller must converge on the same
+ * cached empty mapping instance, and the class must end up flagged as having no mapping.
+ */
+ public void testComputeMappingIfAbsentConvergesOnSameInstanceUnderConcurrencyForUnmappedClass() throws Exception {
+ StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder();
+ ExecutorService pool = Executors.newFixedThreadPool(THREADS);
+ CountDownLatch start = new CountDownLatch(1);
+ List>> futures = new ArrayList<>();
+
+ for (int t = 0; t < THREADS; t++) {
+ futures.add(pool.submit(() -> {
+ start.await();
+ return holder.computeMappingIfAbsent(String.class, clazz -> Collections.emptyMap());
+ }));
+ }
+
+ start.countDown();
+ Map expected = futures.get(0).get(60, TimeUnit.SECONDS);
+ for (Future> future : futures) {
+ assertThat(future.get(60, TimeUnit.SECONDS)).isSameAs(expected).isEmpty();
+ }
+ pool.shutdown();
+
+ assertThat(holder.containsNoMapping(String.class)).isTrue();
+ }
+}
diff --git a/core/src/test/java/org/apache/struts2/conversion/TypeConverterHolderTest.java b/core/src/test/java/org/apache/struts2/conversion/TypeConverterHolderTest.java
new file mode 100644
index 0000000000..9c05997e8b
--- /dev/null
+++ b/core/src/test/java/org/apache/struts2/conversion/TypeConverterHolderTest.java
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.struts2.conversion;
+
+import org.apache.struts2.XWorkTestCase;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * Exercises the {@code default} body of {@link TypeConverterHolder#computeMappingIfAbsent}, the
+ * SPI compatibility fallback kept for third-party {@link TypeConverterHolder} implementations
+ * written before 7.3.0 that do not override the method. {@link StrutsTypeConverterHolder} - the
+ * only in-tree implementation - does override it, so nothing else in the codebase exercises the
+ * default body; these tests do so directly against a minimal implementation that deliberately
+ * implements only the original (non-default) primitives, using plain, non-concurrent collections,
+ * mirroring what a pre-7.3.0 holder looks like.
+ */
+public class TypeConverterHolderTest extends XWorkTestCase {
+
+ /**
+ * Minimal, non-thread-safe {@link TypeConverterHolder} implementing only the pre-7.3.0
+ * primitives. Deliberately does not override {@link TypeConverterHolder#computeMappingIfAbsent},
+ * so calls against it run the interface's default check-then-act body.
+ */
+ private static class LegacyTypeConverterHolder implements TypeConverterHolder {
+ private final Map defaultMappings = new HashMap<>();
+ private final Map> mappings = new HashMap<>();
+ private final Set noMapping = new HashSet<>();
+ private final Set unknownMappings = new HashSet<>();
+
+ @Override
+ public void addDefaultMapping(String className, TypeConverter typeConverter) {
+ defaultMappings.put(className, typeConverter);
+ }
+
+ @Override
+ public boolean containsDefaultMapping(String className) {
+ return defaultMappings.containsKey(className);
+ }
+
+ @Override
+ public TypeConverter getDefaultMapping(String className) {
+ return defaultMappings.get(className);
+ }
+
+ @Override
+ public Map getMapping(Class clazz) {
+ return mappings.get(clazz);
+ }
+
+ @Override
+ public void addMapping(Class clazz, Map mapping) {
+ mappings.put(clazz, mapping);
+ }
+
+ @Override
+ public boolean containsNoMapping(Class clazz) {
+ return noMapping.contains(clazz);
+ }
+
+ @Override
+ public void addNoMapping(Class clazz) {
+ noMapping.add(clazz);
+ }
+
+ @Override
+ public boolean containsUnknownMapping(String className) {
+ return unknownMappings.contains(className);
+ }
+
+ @Override
+ public void addUnknownMapping(String className) {
+ unknownMappings.add(className);
+ }
+ }
+
+ public void testDefaultComputeMappingIfAbsentBuildsOnceAndCaches() {
+ LegacyTypeConverterHolder holder = new LegacyTypeConverterHolder();
+ AtomicInteger builds = new AtomicInteger();
+
+ Map first = holder.computeMappingIfAbsent(String.class, clazz -> {
+ builds.incrementAndGet();
+ Map built = new HashMap<>();
+ built.put("someProperty", "someConverter");
+ return built;
+ });
+ Map second = holder.computeMappingIfAbsent(String.class, clazz -> {
+ throw new AssertionError("builder must not run again once the mapping is cached");
+ });
+
+ assertThat(builds.get()).isEqualTo(1);
+ assertThat(first).containsEntry("someProperty", "someConverter");
+ assertThat(second).isSameAs(first);
+ }
+
+ public void testDefaultComputeMappingIfAbsentNegativeCachesEmptyResult() {
+ LegacyTypeConverterHolder holder = new LegacyTypeConverterHolder();
+ AtomicInteger builds = new AtomicInteger();
+
+ Map result = holder.computeMappingIfAbsent(String.class, clazz -> {
+ builds.incrementAndGet();
+ return new HashMap<>();
+ });
+
+ assertThat(result).isNotNull().isEmpty();
+ assertThat(builds.get()).as("an empty build result must be negative cached").isEqualTo(1);
+ assertThat(holder.containsNoMapping(String.class)).isTrue();
+ }
+
+ public void testDefaultComputeMappingIfAbsentNegativeCachesNullResult() {
+ LegacyTypeConverterHolder holder = new LegacyTypeConverterHolder();
+
+ Map result = holder.computeMappingIfAbsent(String.class, clazz -> null);
+
+ assertThat(result).isNotNull().isEmpty();
+ assertThat(holder.containsNoMapping(String.class)).isTrue();
+ }
+
+ public void testDefaultComputeMappingIfAbsentShortCircuitsOnKnownNoMapping() {
+ LegacyTypeConverterHolder holder = new LegacyTypeConverterHolder();
+ holder.addNoMapping(String.class);
+
+ Map result = holder.computeMappingIfAbsent(String.class, clazz -> {
+ throw new AssertionError("builder must not run for a class already flagged as having no mapping");
+ });
+
+ assertThat(result).isNotNull().isEmpty();
+ }
+}
diff --git a/core/src/test/java/org/apache/struts2/conversion/impl/XWorkConverterTest.java b/core/src/test/java/org/apache/struts2/conversion/impl/XWorkConverterTest.java
index 39fe8e2cf5..9be7c8c482 100644
--- a/core/src/test/java/org/apache/struts2/conversion/impl/XWorkConverterTest.java
+++ b/core/src/test/java/org/apache/struts2/conversion/impl/XWorkConverterTest.java
@@ -19,6 +19,7 @@
package org.apache.struts2.conversion.impl;
import org.apache.struts2.ActionContext;
+import org.apache.struts2.FileManager;
import org.apache.struts2.ModelDrivenAction;
import org.apache.struts2.SimpleAction;
import org.apache.struts2.text.StubTextProvider;
@@ -37,8 +38,11 @@
import org.apache.struts2.util.reflection.ReflectionContextState;
import ognl.OgnlRuntime;
import org.apache.struts2.conversion.TypeConverter;
+import org.apache.struts2.conversion.StrutsTypeConverterHolder;
import java.io.IOException;
+import java.io.InputStream;
+import java.lang.reflect.Field;
import java.math.BigDecimal;
import java.math.BigInteger;
import java.net.URL;
@@ -47,6 +51,12 @@
import java.text.ParseException;
import java.text.SimpleDateFormat;
import java.util.*;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
import static org.junit.Assert.assertArrayEquals;
@@ -804,6 +814,248 @@ public void testCollectionConversion() {
assertEquals(converted, Arrays.asList(1, 2, 3));
}
+ public static class CountingXWorkConverter extends XWorkConverter {
+ final AtomicInteger builds = new AtomicInteger();
+
+ public CountingXWorkConverter() {
+ super();
+ }
+
+ @Override
+ protected Map buildConverterMapping(Class clazz) throws Exception {
+ builds.incrementAndGet();
+ return super.buildConverterMapping(clazz);
+ }
+ }
+
+ public void testGetConverterBuildsMappingExactlyOncePerClass() {
+ CountingXWorkConverter countingConverter = container.inject(CountingXWorkConverter.class);
+ // a cold, dedicated holder so other tests' cached mappings cannot mask the behaviour
+ countingConverter.setTypeConverterHolder(new StrutsTypeConverterHolder());
+
+ Object first = countingConverter.getConverter(User.class, "Collection_list");
+ Object second = countingConverter.getConverter(User.class, "Collection_list");
+
+ assertEquals(String.class, first);
+ assertEquals(String.class, second);
+ assertEquals(1, countingConverter.builds.get());
+ }
+
+ /**
+ * {@code computeMappingIfAbsent} deliberately does not run the builder inside a lock (see
+ * {@link org.apache.struts2.conversion.StrutsTypeConverterHolder#computeMappingIfAbsent}), so
+ * under concurrent first access {@code buildConverterMapping} may run more than once. What must
+ * still hold is that every caller converges on the same, correct result.
+ */
+ public void testGetConverterConvergesOnSameResultUnderConcurrency() throws Exception {
+ final int threads = 16;
+ CountingXWorkConverter countingConverter = container.inject(CountingXWorkConverter.class);
+ countingConverter.setTypeConverterHolder(new StrutsTypeConverterHolder());
+
+ ExecutorService pool = Executors.newFixedThreadPool(threads);
+ CountDownLatch start = new CountDownLatch(1);
+ List> futures = new ArrayList<>();
+
+ for (int t = 0; t < threads; t++) {
+ futures.add(pool.submit(() -> {
+ start.await();
+ return countingConverter.getConverter(User.class, "Collection_list");
+ }));
+ }
+
+ start.countDown();
+ for (Future