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 f1a29d560ffd1d3d2be49954449481b61196028d Author: Lukasz Lenart <[email protected]> AuthorDate: Tue Jul 21 17:51:41 2026 +0200 WW-5539 Remove global lock from DefaultActionValidatorManager getValidators() was synchronized on the singleton manager, so every validated request in the application serialised on it - and the lock covered the per-request Validator construction loop, which operates on per-request objects and never needed mutual exclusion. Both caches become ConcurrentHashMap and cached config lists are wrapped unmodifiable, since several threads now iterate them concurrently. --- .../validator/DefaultActionValidatorManager.java | 63 +++++++++++++--------- .../DefaultActionValidatorManagerTest.java | 44 +++++++++++++++ 2 files changed, 82 insertions(+), 25 deletions(-) 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 2f2b758b7..530b89599 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.net.URL; 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; /** * <p> @@ -67,8 +65,8 @@ public class DefaultActionValidatorManager implements ActionValidatorManager { */ protected static final String VALIDATION_CONFIG_SUFFIX = "-validation.xml"; - protected final Map<String, List<ValidatorConfig>> validatorCache = synchronizedMap(new HashMap<>()); - protected final Map<String, List<ValidatorConfig>> validatorFileCache = synchronizedMap(new HashMap<>()); + protected final Map<String, List<ValidatorConfig>> validatorCache = new ConcurrentHashMap<>(); + protected final Map<String, List<ValidatorConfig>> validatorFileCache = new ConcurrentHashMap<>(); private static final Logger LOG = LogManager.getLogger(DefaultActionValidatorManager.class); protected ValidatorFactory validatorFactory; @@ -137,17 +135,19 @@ public class DefaultActionValidatorManager implements ActionValidatorManager { } @Override - public synchronized List<Validator> getValidators(Class<?> clazz, String context, String method) { + public List<Validator> 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<ValidatorConfig> 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<ValidatorConfig> configs = validatorCache.get(validatorKey); List<Validator> validators = new ArrayList<>(); for (ValidatorConfig config : configs) { if (method == null || method.equals(config.getParams().get("methodName"))) { @@ -158,7 +158,7 @@ public class DefaultActionValidatorManager implements ActionValidatorManager { } @Override - public synchronized List<Validator> getValidators(Class<?> clazz, String context) { + public List<Validator> getValidators(Class<?> clazz, String context) { return getValidators(clazz, context, null); } @@ -314,7 +314,7 @@ public class DefaultActionValidatorManager implements ActionValidatorManager { } checked.add(clazz.getName()); - return validatorConfigs; + return Collections.unmodifiableList(validatorConfigs); } protected List<ValidatorConfig> buildAliasValidatorConfigs(Class<?> aClass, String context, boolean checkFile) { @@ -328,22 +328,35 @@ public class DefaultActionValidatorManager implements ActionValidatorManager { } protected List<ValidatorConfig> loadFile(String fileName, Class<?> clazz, boolean checkFile) { - List<ValidatorConfig> 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<ValidatorConfig> 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<ValidatorConfig> parseValidatorConfigs(URL fileUrl, String fileName) { + List<ValidatorConfig> 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/test/java/org/apache/struts2/validator/DefaultActionValidatorManagerTest.java b/core/src/test/java/org/apache/struts2/validator/DefaultActionValidatorManagerTest.java index 06914d31f..041ca7647 100644 --- a/core/src/test/java/org/apache/struts2/validator/DefaultActionValidatorManagerTest.java +++ b/core/src/test/java/org/apache/struts2/validator/DefaultActionValidatorManagerTest.java @@ -27,6 +27,8 @@ import org.apache.struts2.interceptor.ValidationAware; import org.apache.struts2.test.DataAware2; import org.apache.struts2.test.SimpleAction3; import org.apache.struts2.test.User; +import org.apache.struts2.util.ValueStack; +import org.apache.struts2.util.ValueStackFactory; import org.apache.struts2.validator.validators.DateRangeFieldValidator; import org.apache.struts2.validator.validators.DoubleRangeFieldValidator; import org.apache.struts2.validator.validators.ExpressionValidator; @@ -43,6 +45,11 @@ import java.util.ArrayList; import java.util.Iterator; import java.util.List; import java.util.Map; +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; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -375,4 +382,41 @@ public class DefaultActionValidatorManagerTest extends XWorkTestCase { assertEquals((e.getValue()).get(0), "password hint is required"); } + public void testConcurrentGetValidatorsReturnsConsistentResults() throws Exception { + final int threads = 16; + ExecutorService pool = Executors.newFixedThreadPool(threads); + CountDownLatch start = new CountDownLatch(1); + List<Future<Integer>> futures = new ArrayList<>(); + + int expectedCount = actionValidatorManager.getValidators(SimpleAction.class, alias).size(); + // ActionContext is a plain (non-inheritable) ThreadLocal, so each pool thread needs its + // own context bound - with its own ValueStack - before it can call getValidators(), + // mirroring what XWorkTestCaseHelper does for the main test thread. + ValueStackFactory valueStackFactory = container.getInstance(ValueStackFactory.class); + + for (int t = 0; t < threads; t++) { + futures.add(pool.submit(() -> { + ValueStack stack = valueStackFactory.createValueStack(); + stack.getActionContext().withContainer(container).withValueStack(stack).bind(); + start.await(); + return actionValidatorManager.getValidators(SimpleAction.class, alias).size(); + })); + } + + start.countDown(); + for (Future<Integer> future : futures) { + assertThat(future.get(60, TimeUnit.SECONDS)).isEqualTo(expectedCount); + } + pool.shutdown(); + } + + public void testCachedValidatorConfigsAreUnmodifiable() { + actionValidatorManager.getValidators(SimpleAction.class, alias); + + List<ValidatorConfig> cached = actionValidatorManager.validatorCache.values().iterator().next(); + + assertThatThrownBy(() -> cached.add(null)) + .isInstanceOf(UnsupportedOperationException.class); + } + }
