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 47ce7a8d3805054a1df64dab30feb28ffbc0aba5 Author: Lukasz Lenart <[email protected]> AuthorDate: Tue Jul 21 16:57:05 2026 +0200 WW-5539 Make StrutsTypeConverterHolder collections concurrent The holder is a container singleton whose HashMaps were read without any lock by XWorkConverter.lookup() while being written elsewhere, risking lost updates and torn reads during resize. Null TypeConverters are now ignored with a warning rather than stored, since ConcurrentHashMap forbids null values and a null converter left the holder in an inconsistent state. --- .../conversion/StrutsTypeConverterHolder.java | 24 +++- .../conversion/StrutsTypeConverterHolderTest.java | 133 +++++++++++++++++++++ 2 files changed, 151 insertions(+), 6 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 3013c3bd4..09f5766bc 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,20 @@ */ package org.apache.struts2.conversion; -import java.util.HashMap; -import java.util.HashSet; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + import java.util.Map; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; /** * 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> @@ -34,7 +39,7 @@ public class StrutsTypeConverterHolder implements TypeConverterHolder { * - 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,23 +60,30 @@ public class StrutsTypeConverterHolder implements TypeConverterHolder { * 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. */ - private final HashSet<Class> noMapping = new HashSet<>(); // action + private final Set<Class> noMapping = ConcurrentHashMap.newKeySet(); // action /** * Record classes that doesn't have conversion mapping defined. * <pre> * - String -> classname as String * </pre> + * + * @deprecated since 7.3.0, this field is an implementation detail and will be made private. */ - protected HashSet<String> unknownMappings = new HashSet<>(); // non-action (eg. returned value) + @Deprecated + protected final Set<String> unknownMappings = ConcurrentHashMap.newKeySet(); // non-action (eg. returned value) @Override public void addDefaultMapping(String className, TypeConverter typeConverter) { + if (typeConverter == null) { + LOG.warn("Ignoring null TypeConverter registered for class [{}]", className); + return; + } defaultMappings.put(className, typeConverter); unknownMappings.remove(className); } 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 000000000..6fc2eac80 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java @@ -0,0 +1,133 @@ +/* + * 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.ArrayList; +import java.util.List; +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 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<Future<?>> 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 testConcurrentNoMappingAndUnknownMappingRegistrationLosesNothing() throws Exception { + StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder(); + ExecutorService pool = Executors.newFixedThreadPool(THREADS); + CountDownLatch start = new CountDownLatch(1); + List<Future<?>> 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(); + } +}
