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 c9e6c17ea0b8eb831c6e908374c1d3830e9a26c5 Author: Lukasz Lenart <[email protected]> AuthorDate: Tue Jul 21 17:38:48 2026 +0200 WW-5539 Remove coarse locks from XWorkConverter getConverter() synchronized on the Class object being converted, which is a globally visible monitor any other library may contend on, and which serialised every conversion for a given action class including cache hits. It now delegates to TypeConverterHolder#computeMappingIfAbsent. registerConverter and registerConverterNotFound drop their synchronized modifier; they are single delegations to a concurrent map, and the lock never covered the readers in lookup() in any case. buildConverterMapping no longer stores its result - storage is owned by computeMappingIfAbsent. --- .../struts2/conversion/impl/XWorkConverter.java | 71 +++++++++++---------- .../conversion/impl/XWorkConverterTest.java | 74 ++++++++++++++++++++++ 2 files changed, 113 insertions(+), 32 deletions(-) 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 89d3ecceb..58a075efd 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,41 @@ public class XWorkConverter extends DefaultTypeConverter { } 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); - } - - 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); + if (property == null) { + return null; + } + try { + Map<String, Object> mapping = converterHolder.computeMappingIfAbsent(clazz, this::buildConverterMappingUnchecked); + 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 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 +470,11 @@ public class XWorkConverter extends DefaultTypeConverter { } } - 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 +554,8 @@ public class XWorkConverter extends DefaultTypeConverter { * @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,15 +577,10 @@ public class XWorkConverter extends DefaultTypeConverter { curClazz = curClazz.getSuperclass(); } - if (!mapping.isEmpty()) { - converterHolder.addMapping(clazz, mapping); - } else { - converterHolder.addNoMapping(clazz); - } - return mapping; } + @SuppressWarnings("deprecation") private Map<String, Object> conditionalReload(Class clazz, Map<String, Object> oldValues) throws Exception { Map<String, Object> mapping = oldValues; @@ -584,6 +588,9 @@ public class XWorkConverter extends DefaultTypeConverter { URL fileUrl = ClassLoaderUtil.getResource(buildConverterFilename(clazz), clazz); if (fileManager.fileNeedsReloading(fileUrl)) { mapping = buildConverterMapping(clazz); + // addMapping is deprecated but remains the correct primitive here: + // computeMappingIfAbsent cannot express an unconditional overwrite. + converterHolder.addMapping(clazz, mapping); } } 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 39fe8e2cf..14aeb3905 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 @@ -37,6 +37,8 @@ import org.apache.struts2.util.ValueStack; import org.apache.struts2.util.reflection.ReflectionContextState; import ognl.OgnlRuntime; import org.apache.struts2.conversion.TypeConverter; +import org.apache.struts2.conversion.TypeConverterHolder; +import org.apache.struts2.conversion.StrutsTypeConverterHolder; import java.io.IOException; import java.math.BigDecimal; @@ -47,6 +49,12 @@ import java.text.DateFormat; 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 +812,72 @@ public class XWorkConverterTest extends XWorkTestCase { assertEquals(converted, Arrays.asList(1, 2, 3)); } + public static class CountingXWorkConverter extends XWorkConverter { + final AtomicInteger builds = new AtomicInteger(); + + public CountingXWorkConverter() { + super(); + } + + @Override + protected Map<String, Object> buildConverterMapping(Class clazz) throws Exception { + builds.incrementAndGet(); + return super.buildConverterMapping(clazz); + } + } + + public void testGetConverterBuildsMappingExactlyOncePerClass() throws Exception { + 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()); + } + + public void testGetConverterBuildsMappingExactlyOnceUnderConcurrency() 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<Future<Object>> 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<Object> future : futures) { + assertEquals(String.class, future.get(60, TimeUnit.SECONDS)); + } + pool.shutdown(); + + assertEquals(1, countingConverter.builds.get()); + } + + public void testGetConverterReturnsNullForUnknownProperty() { + XWorkConverter freshConverter = container.inject(XWorkConverter.class); + freshConverter.setTypeConverterHolder(new StrutsTypeConverterHolder()); + + assertNull(freshConverter.getConverter(User.class, "noSuchPropertyAnywhere")); + } + + public void testGetConverterReturnsNullForNullProperty() { + XWorkConverter freshConverter = container.inject(XWorkConverter.class); + freshConverter.setTypeConverterHolder(new StrutsTypeConverterHolder()); + + assertNull(freshConverter.getConverter(User.class, null)); + } + public static class Foo1 { public Bar1 getBar() { return new Bar1Impl();
