From 3cdae67c6384cf1f970ee8677a31500c83e40fe1 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Tue, 21 Jul 2026 16:30:13 +0200 Subject: [PATCH 01/20] WW-5539 docs: add concurrency performance enhancements design Design for removing coarse locks from XWorkConverter, DefaultActionValidatorManager and StrutsTypeConverterHolder in favour of concurrent collections. Co-Authored-By: Claude Opus 4.8 --- ...-WW-5539-concurrency-performance-design.md | 326 ++++++++++++++++++ 1 file changed, 326 insertions(+) create mode 100644 docs/superpowers/specs/2026-07-21-WW-5539-concurrency-performance-design.md diff --git a/docs/superpowers/specs/2026-07-21-WW-5539-concurrency-performance-design.md b/docs/superpowers/specs/2026-07-21-WW-5539-concurrency-performance-design.md new file mode 100644 index 0000000000..2482af6bba --- /dev/null +++ b/docs/superpowers/specs/2026-07-21-WW-5539-concurrency-performance-design.md @@ -0,0 +1,326 @@ +# WW-5539 — Concurrency performance enhancements + +**Jira**: [WW-5539](https://issues.apache.org/jira/browse/WW-5539) +**Fix version**: 7.3.0 +**Component**: Core +**Date**: 2026-07-21 + +## Problem + +WW-5539 names three classes as candidates for improved locking, without further detail. +Inspection confirms a distinct problem in each. + +### `StrutsTypeConverterHolder` — unsynchronised shared mutable state + +All four collections are plain `HashMap`/`HashSet` (`StrutsTypeConverterHolder.java:37-71`). +The holder is a container singleton mutated at runtime through `addDefaultMapping`, +`addMapping`, `addNoMapping` and `addUnknownMapping`. Meanwhile `XWorkConverter.lookup()` +(`XWorkConverter.java:364-368`) reads it under no lock at all, while `registerConverter()` +writes under the `XWorkConverter` monitor. Readers therefore race writers on a `HashMap`: +lost updates, and torn reads during resize. + +This is a correctness defect, not only a contention problem. + +### `XWorkConverter` — two coarse locks + +1. `getConverter()` wraps its body in `synchronized (clazz)` (`XWorkConverter.java:417`). + Locking on a `Class` object is a well-known anti-pattern — the monitor is globally + visible and any other library may contend on it. It also serialises every conversion + for a given action class, including pure cache hits. +2. `registerConverter` and `registerConverterNotFound` are `synchronized` methods + (`XWorkConverter.java:466,470`) on the singleton, so every cache miss takes a + process-wide lock. + +### `DefaultActionValidatorManager` — global lock on the request path + +`getValidators(...)` is `synchronized` on the singleton manager +(`DefaultActionValidatorManager.java:140`). Every validated request in the application +serialises on it. The lock covers not just the cache lookup but the per-request +`Validator` instantiation loop (lines 149-157), which operates on per-request objects +and never needed mutual exclusion. The caches are `synchronizedMap` wrappers with +non-atomic `containsKey`/`get`/`put` sequences layered on top (lines 143-150, 335-347). + +## Approach + +Make the caches genuinely concurrent, then delete the coarse locks. Where a computation +is expensive and provably safe to guard, use `computeIfAbsent`. Where it is not safe, +accept that two threads may occasionally redo cheap idempotent work and converge on the +same answer. + +These caches are read-mostly with a small warm-up burst, which is the workload +`ConcurrentHashMap` is built for. The resulting diff mostly removes code, which matters +for a change whose entire risk profile is the soundness of its concurrency reasoning. + +Two alternatives were considered and rejected: + +- **Striped per-key locking** (interning a lock object per class). Preserves + exactly-once computation everywhere, but adds a lock-object cache that itself needs + eviction reasoning, still blocks readers behind writers, and duplicates the + classloader-pinning problem in a second map. `computeIfAbsent` provides the same + guarantee where it matters, for free. +- **Copy-on-write immutable snapshots.** Fastest possible reads, but `mappings` is keyed + by every action class in the application, so each cold miss copies the whole map: + O(n) per write, O(n²) to warm. Under `struts.configuration.xml.reload` writes never + stop. Wrong shape for this data. + +## Compatibility constraints + +`TypeConverterHolder` is an SPI: aliased to `struts.converter.holder` +(`StrutsBeanSelectionProvider.java:413`) and bound in `struts-beans.xml:115`. Third +parties can supply their own implementations. 7.3.0 is a minor release, so the interface +may gain `default` methods but must not break existing implementations. + +## Design + +### 1. `TypeConverterHolder` SPI + +One new `default` method: + +```java +default Map computeMappingIfAbsent(Class clazz, + Function> builder) +``` + +Contract: return the class's property-converter mapping, building and caching it on first +use. Returns `Collections.emptyMap()` when the class is known to have none — never +`null`. A builder returning `null` or an empty map means "no mapping", and the holder +records that in the negative cache itself. + +The `default` body implements this with the existing +`containsNoMapping`/`getMapping`/`addMapping`/`addNoMapping` primitives — check-then-act, +matching today's semantics. Third-party holders inherit it unchanged and keep working, +without the atomicity benefit. + +Because the `default` body calls methods this same change deprecates, it carries +`@SuppressWarnings("deprecation")`. That is intentional and not an oversight: the fallback +path must keep using the old primitives, since those are the only methods a third-party +implementation is guaranteed to provide. + +`StrutsTypeConverterHolder` overrides it: + +```java +if (noMapping.contains(clazz)) { + return Collections.emptyMap(); +} +Map mapping = mappings.computeIfAbsent(clazz, c -> { + Map built = builder.apply(c); + return (built == null || built.isEmpty()) ? null : built; +}); +if (mapping == null) { + noMapping.add(clazz); + return Collections.emptyMap(); +} +return mapping; +``` + +A builder returning `null` inside `computeIfAbsent` stores nothing and yields `null`, so +the negative case falls out naturally and `getMapping()` retains its current +"null when absent" meaning. + +**Fields** become `ConcurrentHashMap` and `ConcurrentHashMap.newKeySet()`. This is the +part that fixes the data race. + +**Deprecations.** `getMapping`, `addMapping` and `containsNoMapping` are marked +`@Deprecated`: all three are strictly subsumed by `computeMappingIfAbsent`, and each +invites check-then-act at call sites. + +`addNoMapping` is **not** deprecated. `XWorkConverter.getConverter` catches `Throwable` +and negative-caches the failure; that is a distinct operation ("this class failed to +build, stop retrying"), and folding it into the compute method would mean swallowing +`Throwable` inside the SPI, hiding real failures from implementers. + +The default-mapping methods (`addDefaultMapping`, `containsDefaultMapping`, +`getDefaultMapping`, `containsUnknownMapping`, `addUnknownMapping`) are **not** +deprecated. They back the cache that cannot use `computeIfAbsent` (see below) and have +live external callers in `StrutsConversionPropertiesProcessor` and +`DefaultConversionAnnotationProcessor`. + +Removal of the deprecated methods is tracked separately by the project lead. + +**The `protected HashSet unknownMappings` field** is retyped to `Set`, +made `final`, backed by `ConcurrentHashMap.newKeySet()`, and marked `@Deprecated`. +Retyping is a source-compatibility break for any subclass that assigns it or calls a +`HashSet`-specific method. The risk is judged negligible, but it is a break and is +recorded here deliberately rather than left implicit. + +### 2. `XWorkConverter` + +**`getConverter(Class, String)` — `synchronized (clazz)` removed:** + +```java +protected Object getConverter(Class clazz, String property) { + if (property == null) { + return null; + } + try { + Map mapping = converterHolder.computeMappingIfAbsent(clazz, this::buildConverterMapping); + mapping = conditionalReload(clazz, mapping); + return mapping.get(property); + } catch (Throwable t) { + LOG.debug("Got exception trying to resolve converter for class [{}] and property [{}]", clazz, property, t); + converterHolder.addNoMapping(clazz); + return null; + } +} +``` + +The `containsNoMapping` guard folds into the holder: a negative-cached class returns an +empty map and `mapping.get(property)` yields `null`, the same outcome. The +`catch (Throwable)` behaviour is preserved exactly. + +Two consequences: + +- **`buildConverterMapping` stops storing.** It currently calls `addMapping`/`addNoMapping` + itself (`XWorkConverter.java:571-575`); with the holder owning storage that becomes a + double write. It reduces to "build and return the map". The method is `protected`, so + this is a behaviour change visible to subclasses and belongs in the release notes. +- **`buildConverterMapping` declares `throws Exception`**, which does not fit `Function`. + It is wrapped in a private lambda that rethrows as unchecked; the outer + `catch (Throwable)` still catches it, so negative-caching semantics are unchanged. + +**`conditionalReload` stays outside the compute.** It only does work when +`struts.configuration.xml.reload` is enabled, and it must run on cache *hits* — that is +its purpose. Under reload it may rebuild concurrently with last-write-wins; that is a +dev-mode-only path where the current code is no better, and rebuilding is idempotent. + +**`registerConverter` / `registerConverterNotFound` — `synchronized` removed.** Both are +now single delegations to a concurrent map. They are `public`, so the modifier is +externally observable; any caller relying on them for mutual exclusion was relying on a +lock that never covered the readers in `lookup()`. + +**`lookup(String, boolean)` keeps check-then-act.** Its resolver is `lookupSuper()` +(`XWorkConverter.java:600-626`), which *reads* `getDefaultMapping()` recursively while +walking the class hierarchy. A recursive read inside `ConcurrentHashMap.computeIfAbsent` +is forbidden — it deadlocks or throws `IllegalStateException: Recursive update`. This +cache therefore simply becomes lock-free. Two threads racing on a cold miss may both walk +the hierarchy and both call `registerConverter` with an equal result; the work is a few +lock-free map reads and the outcome is identical. + +**Known benign race, deliberately not closed.** `lookup` (line 364) reads +`containsUnknownMapping` and `containsDefaultMapping` as two separate calls. Each is +atomic against the concurrent map, but the *pair* is not — a converter registered between +them yields a stale `null` for that one call. Today, against an unsynchronised `HashMap`, +this is outright unsafe; afterwards it is a benign race that self-corrects on the next +lookup. No lock is added: doing so would reintroduce the contention being removed, on the +hottest read path, to close a window that resolves itself. This is recorded so a future +reader does not mistake it for an oversight. + +### 3. `DefaultActionValidatorManager` + +**Both `getValidators` overloads lose `synchronized`.** The three-argument one becomes: + +```java +String validatorKey = buildValidatorKey(clazz, context); +List configs = validatorCache.get(validatorKey); +if (configs == null) { + configs = validatorCache.computeIfAbsent(validatorKey, + k -> buildValidatorConfigs(clazz, context, false, null)); +} else if (reloadingConfigs) { + configs = buildValidatorConfigs(clazz, context, true, null); + validatorCache.put(validatorKey, configs); +} +``` + +This mirrors the existing `containsKey` / `else if (reloadingConfigs)` logic exactly, +including that a first-ever call builds with `checkFile=false` even in reload mode. + +**The substantive win is what now sits outside any lock:** the loop at lines 149-157 that +reads the `ValueStack` and calls `validatorFactory.getValidator(config)` per config. That +is per-request work on per-request objects. Setting caching aside entirely, removing it +from the critical path takes the manager off the serialisation path of every validated +request. + +**`loadFile` gets the same treatment**, with the file-reload branch kept explicit: + +```java +URL fileUrl = ClassLoaderUtil.getResource(fileName, clazz); +if (checkFile && fileManager.fileNeedsReloading(fileUrl)) { + List reloaded = parseValidatorConfigs(fileUrl, fileName); + validatorFileCache.put(fileName, reloaded); + return reloaded; +} +return validatorFileCache.computeIfAbsent(fileName, k -> parseValidatorConfigs(fileUrl, fileName)); +``` + +`parseValidatorConfigs(URL, String)` is a new private helper extracted from the existing +body of `loadFile` — the `fileManager.loadFile` / `validatorFileParser` / +`catch (IOException)` block at lines 336-342, unchanged in behaviour. It is extracted only +so the same logic can serve both branches above without duplication. + +This `computeIfAbsent` runs inside the one on `validatorCache`. They are different maps +and nothing calls back the other direction, so there is no lock-ordering cycle. Empty +results are still cached, preserving today's negative caching. + +**Both caches become `ConcurrentHashMap`.** The declared field types stay +`Map>` and stay `protected`, so `AnnotationActionValidatorManager` +and any third-party subclass are unaffected. + +**Cached lists are wrapped unmodifiable before publication.** With `synchronized` gone, +several threads iterate the same cached `List` concurrently. That is safe +only while no cached list is mutated after publication — true today, but only incidentally, +and `buildValidatorConfigs` does `addAll` into lists that flow around freely. Wrapping makes +it true by construction: a future mutation fails loudly at the mistake instead of becoming +an intermittent production heisenbug. Callers were checked — +`buildClassValidatorConfigs`/`buildAliasValidatorConfigs` results are only read or +`addAll`-ed into a fresh accumulator, and `AnnotationActionValidatorManager` already copies +defensively. + +**Noted limitation, untouched:** `AnnotationActionValidatorManager.buildValidatorKey` calls +`ActionContext.getContext().getActionInvocation()`, so the cache key depends on per-request +state. It works, but the class cannot be reasoned about purely from its own source. + +## Testing + +**Regression gate.** The existing `XWorkConverterTest`, `AnnotationXWorkConverterTest`, +`DefaultActionValidatorManagerTest` and `AnnotationActionValidatorManagerTest` +(`XWorkTestCase`-based, JUnit 4 as core actually uses) must pass untouched. Any need to +*edit* them signals a semantic change that this design says will not happen. + +**New concurrency tests**, added to the existing test classes. Each uses 16 threads +released simultaneously via `CountDownLatch`, and asserts on collected results rather than +on timing, so there is no sleep-based flakiness: + +- `StrutsTypeConverterHolder`: 16 threads registering distinct default mappings while + others read; assert every registration is visible at the end and no read returns a torn + result. This test would plausibly fail against today's code. +- `XWorkConverter.getConverter`: 16 threads resolving converters for the same class + concurrently; assert equal results and no exception. Additionally assert + `buildConverterMapping` runs exactly once per class via a counting subclass — the + concrete payoff of `computeMappingIfAbsent`. +- `DefaultActionValidatorManager.getValidators`: 16 threads against the same class and + context; assert consistent validator lists and a single build. + +**Caveat.** These tests can demonstrate a race but never its absence; a green run on a +strongly-ordered x86 machine is weak evidence. The primary correctness argument is the +reasoning about the data structures — reads on concurrent collections, both +`computeIfAbsent` call sites provably not re-entering their own maps, and the remaining +races benign and self-correcting. The tests are a backstop for that argument, not a +substitute. + +**Benchmark, not committed.** A throwaway harness measuring `getValidators` and +`getConverter` throughput at 1/4/16/64 threads, before and after, warmed. Numbers go in +the PR description. The result will be reported faithfully, including the realistic +outcome that the converter changes do not move the needle while the validator lock does — +the converter caches warm fast, and the validator lock is the coarser of the two. + +## Out of scope + +1. **Classloader pinning.** `mappings` and `noMapping` hold strong `Class` references in a + container singleton, keeping webapp classloaders alive across hot redeploy. A separate + Jira issue: it is a lifetime/correctness issue rather than a locking one, and fixing it + changes cache semantics. +2. **The `FIXME lukaszlenart` in `TypeConverterHolder`** about merging `unknownMappings` + into `noMapping` — a semantic consolidation, unrelated to locking. +3. **Removal of the deprecated methods** — tracked by the project lead for a later release. + +## Risk + +The change is mostly deletion, but it deletes locks, so the failure mode is silent and +load-dependent rather than a failing build. Two things warrant the closest review: + +- No `computeIfAbsent` builder can re-enter its own map. Verified for both call sites; + the `lookupSuper` case is precisely why the default-mapping cache keeps check-then-act. + Confirmed that `DefaultConversionFileProcessor` never touches the holder and + `DefaultConversionAnnotationProcessor` writes only to `defaultMappings`, a different map. +- No cached collection is mutated after publication — enforced by unmodifiable wrapping in + `DefaultActionValidatorManager`. From be2b8aad01b842493b169b85811a918a5f263c9f Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Tue, 21 Jul 2026 16:40:22 +0200 Subject: [PATCH 02/20] WW-5539 docs: add concurrency performance implementation plan Five tasks derived from the approved design: make StrutsTypeConverterHolder concurrent, add the computeMappingIfAbsent SPI method, remove the locks from XWorkConverter and DefaultActionValidatorManager, then benchmark and raise the PR. Co-Authored-By: Claude Opus 4.8 --- ...6-07-21-WW-5539-concurrency-performance.md | 1168 +++++++++++++++++ 1 file changed, 1168 insertions(+) create mode 100644 docs/superpowers/plans/2026-07-21-WW-5539-concurrency-performance.md diff --git a/docs/superpowers/plans/2026-07-21-WW-5539-concurrency-performance.md b/docs/superpowers/plans/2026-07-21-WW-5539-concurrency-performance.md new file mode 100644 index 0000000000..1637ddd628 --- /dev/null +++ b/docs/superpowers/plans/2026-07-21-WW-5539-concurrency-performance.md @@ -0,0 +1,1168 @@ +# WW-5539 Concurrency Performance Enhancements Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Remove three coarse locks from Struts core's type-conversion and validation caches, replacing them with concurrent collections so that read-mostly cache hits no longer serialise. + +**Architecture:** Make the backing caches genuinely thread-safe (`ConcurrentHashMap` / `ConcurrentHashMap.newKeySet()`), then delete the `synchronized` blocks that were compensating for them. One new `default` SPI method, `TypeConverterHolder.computeMappingIfAbsent`, provides atomic build-once semantics for the single expensive computation where `computeIfAbsent` is provably safe. Everywhere else, check-then-act is retained and the duplicated work is cheap and idempotent. + +**Tech Stack:** Java 17 (`maven.compiler.release=17`), Maven multi-module, JUnit 3-style tests via `junit.framework.TestCase` (through `org.apache.struts2.XWorkTestCase`), AssertJ assertions, Log4j2. + +**Spec:** `docs/superpowers/specs/2026-07-21-WW-5539-concurrency-performance-design.md` + +## Global Constraints + +- **Branch:** all work lands on `WW-5539`. Never commit to `main`. +- **Commit messages:** must be prefixed `WW-5539 `. +- **Target release:** 7.3.0 (minor) — the `TypeConverterHolder` SPI may gain `default` methods but must not break existing third-party implementations. +- **Test style:** core tests extend `org.apache.struts2.XWorkTestCase`, which extends `junit.framework.TestCase`. Test methods are `public void testXxx()` with **no** `@Test` annotation. Setup is `@Override protected void setUp() throws Exception { super.setUp(); ... }`. Do **not** convert these to JUnit 5. +- **Available test fields:** `XWorkTestCase` provides `protected Container container` and `protected ConfigurationManager configurationManager`. +- **`TypeConverter` is a functional interface** (single abstract method `convertValue(Map, Object, Member, String, Object, Class)`), so test stubs may be lambdas. +- **Build command:** `mvn test -DskipAssembly -pl core -Dtest=ClassName#methodName` +- **No binary-compatibility gate** (no japicmp/revapi) and **deprecation warnings do not fail the build** — verified in `pom.xml`. +- **Exactly three methods get `@Deprecated`:** `TypeConverterHolder.getMapping`, `addMapping`, `containsNoMapping`. Do not deprecate `addNoMapping` or any of the five default-mapping methods. + +--- + +### Task 1: Make `StrutsTypeConverterHolder` thread-safe + +This is the correctness fix. The holder is a container singleton whose plain `HashMap`s are read without any lock by `XWorkConverter.lookup()` while being written elsewhere. + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java:21-77` +- Test: `core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java` (create) + +**Interfaces:** +- Consumes: nothing from earlier tasks. +- Produces: `StrutsTypeConverterHolder` with thread-safe internals and a `public StrutsTypeConverterHolder()` no-arg constructor (already implicit). Field `protected final Set unknownMappings`. Behaviour change: `addDefaultMapping(className, null)` is now ignored rather than storing a null value. + +- [ ] **Step 1: Write the failing concurrency test** + +Create `core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java`: + +```java +/* + * 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> 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> 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(); + } +} +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsTypeConverterHolderTest` + +Expected: `testAddDefaultMappingIgnoresNullConverter` FAILS (currently a null value is stored, so `containsDefaultMapping` returns `true`). + +The two concurrency tests are **expected to be flaky against the unfixed code** — they may pass on a strongly-ordered x86/ARM machine. Record whatever happens; do not treat a pass as evidence the current code is correct. The null-converter test is the deterministic gate for this step. + +- [ ] **Step 3: Make the collections concurrent** + +In `StrutsTypeConverterHolder.java`, replace the imports at lines 21-23: + +```java +import java.util.Map; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +``` + +Replace the four field declarations (lines 37, 58, 63, 71) with: + +```java + private final Map defaultMappings = new ConcurrentHashMap<>(); // non-action (eg. returned value) +``` + +```java + private final Map> mappings = new ConcurrentHashMap<>(); // action +``` + +```java + private final Set noMapping = ConcurrentHashMap.newKeySet(); // action +``` + +```java + /** + * Record classes that doesn't have conversion mapping defined. + * 
+     * - String -> classname as String
+     *
+ * + * @deprecated since 7.3.0, this field is an implementation detail and will be made private. + */ + @Deprecated + protected final Set unknownMappings = ConcurrentHashMap.newKeySet(); // non-action (eg. returned value) +``` + +Keep the existing Javadoc comments above `defaultMappings`, `mappings` and `noMapping` unchanged. + +- [ ] **Step 4: Add the null-converter guard** + +`ConcurrentHashMap` forbids null values, so `addDefaultMapping` would now throw `NullPointerException` where it previously stored a null. A null converter was never useful — it left `containsDefaultMapping` returning `true` while `getDefaultMapping` returned `null`. `ConverterFactory` is a pluggable SPI, so a third-party implementation returning null is reachable. Skip it explicitly instead. + +Add the logger imports to `StrutsTypeConverterHolder.java`: + +```java +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +``` + +Add the field just inside the class body, before `defaultMappings`: + +```java + private static final Logger LOG = LogManager.getLogger(StrutsTypeConverterHolder.class); +``` + +Replace `addDefaultMapping` (lines 73-77) with: + +```java + @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); + } +``` + +- [ ] **Step 5: Run the tests to verify they pass** + +Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsTypeConverterHolderTest` + +Expected: PASS, all four tests. + +- [ ] **Step 6: Run the surrounding regression suites** + +Run: `mvn test -DskipAssembly -pl core -Dtest='XWorkConverterTest,AnnotationXWorkConverterTest,StrutsConversionPropertiesProcessorTest,ConfigurationManagerTest'` + +Expected: PASS, no edits to those files. + +- [ ] **Step 7: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java \ + core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java +git commit -m "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." +``` + +--- + +### Task 2: Add `computeMappingIfAbsent` to the `TypeConverterHolder` SPI + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/conversion/TypeConverterHolder.java:19-100` +- Modify: `core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java` +- Test: `core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java` (modify) + +**Interfaces:** +- Consumes: `StrutsTypeConverterHolder` with concurrent internals (Task 1). +- Produces: `Map TypeConverterHolder.computeMappingIfAbsent(Class clazz, Function> builder)` — never returns `null`; returns `Collections.emptyMap()` when the class has no mapping. `getMapping`, `addMapping`, `containsNoMapping` are now `@Deprecated`. + +- [ ] **Step 1: Write the failing tests** + +Append to `StrutsTypeConverterHolderTest.java` (and add these imports): + +```java +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; +``` + +```java + 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(); + } + + 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(); + } + + public void testComputeMappingIfAbsentBuildsOnceUnderConcurrency() throws Exception { + StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder(); + AtomicInteger builds = new AtomicInteger(); + 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 -> { + builds.incrementAndGet(); + 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(builds.get()).as("mapping must be built exactly once").isEqualTo(1); + } +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsTypeConverterHolderTest` + +Expected: COMPILATION FAILURE — `cannot find symbol: method computeMappingIfAbsent`. + +- [ ] **Step 3: Add the `default` method to the SPI** + +In `TypeConverterHolder.java`, add imports: + +```java +import java.util.Collections; +import java.util.Map; +import java.util.function.Function; +``` + +Add `@Deprecated` to exactly three existing methods, keeping their existing Javadoc and adding a `@deprecated` tag to each: + +```java + /** + * Target class conversion Mappings. + * + * @param clazz class to convert to/from + * @return {@link TypeConverter} for given class + * @deprecated since 7.3.0, use {@link #computeMappingIfAbsent(Class, Function)} which resolves + * and caches the mapping atomically instead of requiring a check-then-act at the call site. + */ + @Deprecated + Map getMapping(Class clazz); + + /** + * Assign mapping of converters for given class + * + * @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 + void addMapping(Class clazz, Map mapping); + + /** + * Check if there is no mapping for given class to convert + * + * @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 + boolean containsNoMapping(Class clazz); +``` + +Leave `addNoMapping`, `addDefaultMapping`, `containsDefaultMapping`, `getDefaultMapping`, `containsUnknownMapping` and `addUnknownMapping` **undeprecated**. + +Add the new method at the end of the interface, before the closing brace: + +```java + /** + * 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 + * {@link Collections#emptyMap()}. + * + *

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 are expected to make this atomic so that the builder runs at most once per + * class. 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; + } +``` + +The `@SuppressWarnings("deprecation")` is deliberate: the fallback path must keep using the old primitives, because those are the only methods a third-party implementation is guaranteed to provide. + +- [ ] **Step 4: Override it in `StrutsTypeConverterHolder`** + +Add imports to `StrutsTypeConverterHolder.java`: + +```java +import java.util.Collections; +import java.util.function.Function; +``` + +Add the override after `getMapping`, and mark the three implementing methods `@Deprecated` to match the interface: + +```java + @Override + public Map computeMappingIfAbsent(Class clazz, Function> builder) { + if (noMapping.contains(clazz)) { + return Collections.emptyMap(); + } + Map mapping = mappings.computeIfAbsent(clazz, c -> { + Map built = builder.apply(c); + return (built == null || built.isEmpty()) ? null : built; + }); + if (mapping == null) { + noMapping.add(clazz); + return Collections.emptyMap(); + } + return mapping; + } +``` + +Returning `null` from a `computeIfAbsent` mapping function stores nothing and yields `null`, so the negative case falls out naturally and `getMapping()` keeps its "null when absent" meaning. + +Add `@Deprecated` to the `getMapping`, `addMapping` and `containsNoMapping` overrides in this class so they do not emit "overrides deprecated method" noise. + +- [ ] **Step 5: Run the tests to verify they pass** + +Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsTypeConverterHolderTest` + +Expected: PASS, all nine tests. + +- [ ] **Step 6: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/conversion/TypeConverterHolder.java \ + core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java \ + core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java +git commit -m "WW-5539 Add TypeConverterHolder#computeMappingIfAbsent + +Adds an atomic build-once-and-cache operation so callers no longer need +check-then-act around the class mapping cache, and deprecates the three +primitives it subsumes: getMapping, addMapping and containsNoMapping. + +The method is a default method delegating to those primitives, so +third-party TypeConverterHolder implementations keep working unchanged." +``` + +--- + +### Task 3: Remove the locks from `XWorkConverter` + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/conversion/impl/XWorkConverter.java:414-443` (`getConverter`), `:466-472` (`registerConverter`, `registerConverterNotFound`), `:551-578` (`buildConverterMapping`) +- Test: `core/src/test/java/org/apache/struts2/conversion/impl/XWorkConverterTest.java` (modify) + +**Interfaces:** +- Consumes: `TypeConverterHolder.computeMappingIfAbsent(Class, Function)` (Task 2). +- Produces: `protected Map buildConverterMapping(Class clazz) throws Exception` — **no longer stores** its result in the holder; it only builds and returns. `getConverter`, `registerConverter` and `registerConverterNotFound` are no longer synchronized. + +- [ ] **Step 1: Write the failing test** + +Append to `XWorkConverterTest.java`. Add these imports: + +```java +import org.apache.struts2.conversion.TypeConverterHolder; +import org.apache.struts2.conversion.StrutsTypeConverterHolder; +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; +``` + +Add this static nested class inside `XWorkConverterTest` (it is in package `org.apache.struts2.conversion.impl`, so it can call the protected `XWorkConverter()` constructor): + +```java + 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); + } + } +``` + +And these test methods: + +```java + 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> 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 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)); + } +``` + +`org.apache.struts2.test.User` is already imported by this test class and has a +`User-conversion.properties` declaring `Collection_list = java.lang.String`, so the built +mapping is non-empty and `getConverter` returns `String.class`. + +- [ ] **Step 2: Run the tests and record that they PASS** + +Run: `mvn test -DskipAssembly -pl core -Dtest=XWorkConverterTest` + +Expected: **PASS — all four new tests pass against the unmodified code.** + +This is deliberate and is not a mistake in the plan. Unlike Tasks 1 and 2, there is no failing-test-first cycle available here: the existing `synchronized (clazz)` block already guarantees build-once semantics, and `buildConverterMapping` already caches on the cold path. The old code is *correct*; it is only serialised. These tests exist to pin that correctness down **before** the lock is removed, so that Step 6 proves the refactor preserved it. + +If any of them FAIL at this step, stop and investigate — the current behaviour differs from what the design assumed, and the design needs revisiting before any lock comes out. + +- [ ] **Step 3: Rewrite `getConverter`** + +Replace lines 414-443 of `XWorkConverter.java` with: + +```java + protected Object getConverter(Class clazz, String property) { + LOG.debug("Retrieving converter for class [{}] and property [{}]", clazz, property); + + if (property == null) { + return null; + } + try { + Map 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 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); + } + } +``` + +The `containsNoMapping` guard is gone because the holder now owns it: a negative-cached class returns an empty map and `mapping.get(property)` yields `null`, the same outcome as before. + +- [ ] **Step 4: Stop `buildConverterMapping` from storing** + +Replace lines 571-577 of `XWorkConverter.java` — the tail of `buildConverterMapping` — so it only builds and returns: + +```java + return mapping; + } +``` + +That is, delete this block entirely: + +```java + if (!mapping.isEmpty()) { + converterHolder.addMapping(clazz, mapping); + } else { + converterHolder.addNoMapping(clazz); + } +``` + +Update the method's Javadoc to record the behaviour change, since it is `protected` and visible to subclasses: + +```java + * @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)}. +``` + +- [ ] **Step 5: Drop `synchronized` from the register methods** + +Replace lines 466-472 with: + +```java + public void registerConverter(String className, TypeConverter converter) { + converterHolder.addDefaultMapping(className, converter); + } + + public void registerConverterNotFound(String className) { + converterHolder.addUnknownMapping(className); + } +``` + +Both are now single delegations to a concurrent map. They are `public`, so the modifier change is externally observable — but any caller relying on them for mutual exclusion was relying on a lock that never covered the readers in `lookup()` anyway. + +Leave `lookup(String, boolean)` structurally unchanged. Its resolver `lookupSuper()` reads `getDefaultMapping()` recursively while walking the class hierarchy, and a recursive read inside `ConcurrentHashMap.computeIfAbsent` is forbidden — it deadlocks or throws `IllegalStateException: Recursive update`. It simply becomes lock-free. + +- [ ] **Step 6: Run the tests to verify they pass** + +Run: `mvn test -DskipAssembly -pl core -Dtest=XWorkConverterTest` + +Expected: PASS, including the four new tests and all pre-existing ones with no edits to them. + +- [ ] **Step 7: Run the wider conversion regression suite** + +Run: `mvn test -DskipAssembly -pl core -Dtest='AnnotationXWorkConverterTest,StrutsConversionPropertiesProcessorTest,StrutsTypeConverterHolderTest,ConfigurationManagerTest'` + +Expected: PASS. + +- [ ] **Step 8: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/conversion/impl/XWorkConverter.java \ + core/src/test/java/org/apache/struts2/conversion/impl/XWorkConverterTest.java +git commit -m "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." +``` + +--- + +### Task 4: Remove the global lock from `DefaultActionValidatorManager` + +The expected headline win: today every validated request in the application serialises on this singleton's monitor, and the lock covers per-request `Validator` construction that never needed it. + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/validator/DefaultActionValidatorManager.java:44` (import), `:70-71` (caches), `:139-163` (`getValidators`), `:280-350` (`buildValidatorConfigs`, `loadFile`) +- Test: `core/src/test/java/org/apache/struts2/validator/DefaultActionValidatorManagerTest.java` (modify) + +**Interfaces:** +- Consumes: nothing from earlier tasks. +- Produces: `getValidators(Class, String, String)` and `getValidators(Class, String)` are no longer synchronized. `protected List parseValidatorConfigs(URL fileUrl, String fileName)` is new. Cached `List` values are unmodifiable. + +- [ ] **Step 1: Write the failing test** + +Append to `DefaultActionValidatorManagerTest.java`. Add these imports: + +```java +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; +``` + +```java + public void testConcurrentGetValidatorsReturnsConsistentResults() throws Exception { + final int threads = 16; + ExecutorService pool = Executors.newFixedThreadPool(threads); + CountDownLatch start = new CountDownLatch(1); + List> futures = new ArrayList<>(); + + int expectedCount = actionValidatorManager.getValidators(SimpleAction.class, alias).size(); + + for (int t = 0; t < threads; t++) { + futures.add(pool.submit(() -> { + start.await(); + return actionValidatorManager.getValidators(SimpleAction.class, alias).size(); + })); + } + + start.countDown(); + for (Future future : futures) { + assertThat(future.get(60, TimeUnit.SECONDS)).isEqualTo(expectedCount); + } + pool.shutdown(); + } + + public void testCachedValidatorConfigsAreUnmodifiable() { + actionValidatorManager.getValidators(SimpleAction.class, alias); + + List cached = actionValidatorManager.validatorCache.values().iterator().next(); + + assertThatThrownBy(() -> cached.add(null)) + .isInstanceOf(UnsupportedOperationException.class); + } +``` + +`assertThat` and `assertThatThrownBy` are already statically imported by this test class, and `alias`, `actionValidatorManager`, `SimpleAction` and `ArrayList`/`List` are already available. + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `mvn test -DskipAssembly -pl core -Dtest=DefaultActionValidatorManagerTest` + +Expected: `testCachedValidatorConfigsAreUnmodifiable` FAILS — the cached list is a plain `ArrayList`, so `add(null)` succeeds and no exception is thrown. + +`testConcurrentGetValidatorsReturnsConsistentResults` is expected to PASS at this step; the current code is correct, just serialised. It is a regression guard for the lock removal, not a demonstration of an existing bug. + +- [ ] **Step 3: Make the caches concurrent** + +In `DefaultActionValidatorManager.java`, delete the static import at line 44: + +```java +import static java.util.Collections.synchronizedMap; +``` + +Add: + +```java +import java.util.concurrent.ConcurrentHashMap; +``` + +Replace lines 70-71: + +```java + protected final Map> validatorCache = new ConcurrentHashMap<>(); + protected final Map> validatorFileCache = new ConcurrentHashMap<>(); +``` + +The declared field types stay `Map<...>` and stay `protected`, so `AnnotationActionValidatorManager` and any third-party subclass are unaffected. + +- [ ] **Step 4: Rewrite `getValidators` without the lock** + +Replace lines 139-163 with: + +```java + @Override + public List getValidators(Class clazz, String context, String method) { + String validatorKey = buildValidatorKey(clazz, context); + + List configs = validatorCache.get(validatorKey); + if (configs == null) { + configs = validatorCache.computeIfAbsent(validatorKey, + key -> buildValidatorConfigs(clazz, context, false, null)); + } else if (reloadingConfigs) { + configs = buildValidatorConfigs(clazz, context, true, null); + validatorCache.put(validatorKey, configs); + } + + ValueStack stack = ActionContext.getContext().getValueStack(); + List validators = new ArrayList<>(); + for (ValidatorConfig config : configs) { + if (method == null || method.equals(config.getParams().get("methodName"))) { + validators.add(getValidatorFromValidatorConfig(config, stack)); + } + } + return validators; + } + + @Override + public List getValidators(Class clazz, String context) { + return getValidators(clazz, context, null); + } +``` + +This mirrors the previous `containsKey` / `else if (reloadingConfigs)` logic exactly, including that a first-ever call builds with `checkFile=false` even in reload mode. The `Validator` construction loop is now outside any lock — it is per-request work on per-request objects. + +- [ ] **Step 5: Make `buildValidatorConfigs` return an unmodifiable list** + +Replace the final `return validatorConfigs;` at line 317 with: + +```java + return Collections.unmodifiableList(validatorConfigs); +``` + +`java.util.Collections` is already imported. With `synchronized` gone, several threads now iterate the same cached list concurrently; that is safe only while no cached list is mutated after publication. Wrapping makes it true by construction rather than by accident. + +- [ ] **Step 6: Rewrite `loadFile` and extract `parseValidatorConfigs`** + +Replace lines 330-350 with: + +```java + protected List loadFile(String fileName, Class clazz, boolean checkFile) { + URL fileUrl = ClassLoaderUtil.getResource(fileName, clazz); + + if (checkFile && fileManager.fileNeedsReloading(fileUrl)) { + List reloaded = parseValidatorConfigs(fileUrl, fileName); + validatorFileCache.put(fileName, reloaded); + return reloaded; + } + + 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; + } +``` + +This `computeIfAbsent` runs inside the one on `validatorCache` (via `buildValidatorConfigs`). They are different maps and nothing calls back the other direction, so there is no lock-ordering cycle. Empty results are still cached, preserving today's negative caching. + +- [ ] **Step 7: Run the tests to verify they pass** + +Run: `mvn test -DskipAssembly -pl core -Dtest=DefaultActionValidatorManagerTest` + +Expected: PASS, including both new tests and all pre-existing ones with no edits to them. + +- [ ] **Step 8: Run the validator regression suite** + +Run: `mvn test -DskipAssembly -pl core -Dtest='ActionValidatorManagerTest,AnnotationActionValidatorManagerTest,DefaultActionValidatorManagerTest'` + +Expected: PASS. `AnnotationActionValidatorManagerTest` is the important one — it exercises the wildcard/alias key logic and the subclass that overrides `buildClassValidatorConfigs`. + +- [ ] **Step 9: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/validator/DefaultActionValidatorManager.java \ + core/src/test/java/org/apache/struts2/validator/DefaultActionValidatorManagerTest.java +git commit -m "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." +``` + +--- + +### Task 5: Full build, benchmark, and pull request + +**Files:** +- Create: `/ww5539-benchmark/` — throwaway, **not** committed +- Modify: none + +**Interfaces:** +- Consumes: all changes from Tasks 1-4. +- Produces: before/after throughput numbers for the PR description. + +- [ ] **Step 1: Run the full core test suite** + +Run: `mvn test -DskipAssembly -pl core` + +Expected: BUILD SUCCESS. If anything fails, fix it before proceeding — a green core suite is the gate for this change, since the failure mode of a bad lock removal is silent and load-dependent rather than a compile error. + +- [ ] **Step 2: Run the full multi-module build** + +Run: `mvn test -DskipAssembly` + +Expected: BUILD SUCCESS. This covers the plugins, which is where a third-party-style `TypeConverterHolder` or validator subclass would surface. + +- [ ] **Step 3: Write the throwaway benchmark** + +This runs as a **temporary test class** so it inherits the container bootstrap from `XWorkTestCase` — standing up a Struts container by hand is far more work than the benchmark is worth. It is deleted in Step 5 and never committed. + +Create `core/src/test/java/org/apache/struts2/validator/Ww5539BenchmarkTest.java`: + +```java +package org.apache.struts2.validator; + +import org.apache.struts2.SimpleAction; +import org.apache.struts2.XWorkTestCase; +import org.apache.struts2.conversion.impl.XWorkConverter; +import org.apache.struts2.test.User; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.Callable; +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; + +public class Ww5539BenchmarkTest extends XWorkTestCase { + + private static final int[] THREAD_COUNTS = {1, 4, 16, 64}; + private static final int WARMUP_ITERATIONS = 20_000; + private static final long MEASURE_MILLIS = 3_000; + + public void testBenchmarkValidatorManager() throws Exception { + DefaultActionValidatorManager manager = container.inject(DefaultActionValidatorManager.class); + benchmark("getValidators", () -> manager.getValidators(SimpleAction.class, "validationAlias").size()); + } + + public void testBenchmarkConverter() throws Exception { + XWorkConverter converter = container.getInstance(XWorkConverter.class); + benchmark("getConverter", () -> converter.convertValue(null, new User(), null, "Collection_list", "1", String.class)); + } + + private void benchmark(String label, Callable op) throws Exception { + for (int i = 0; i < WARMUP_ITERATIONS; i++) { + op.call(); + } + System.out.println("=== " + label + " ==="); + for (int threads : THREAD_COUNTS) { + System.out.printf("%s threads=%d ops/sec=%,d%n", label, threads, measure(op, threads)); + } + } + + private long measure(Callable op, int threads) throws Exception { + ExecutorService pool = Executors.newFixedThreadPool(threads); + CountDownLatch start = new CountDownLatch(1); + long deadline = System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(MEASURE_MILLIS); + List> futures = new ArrayList<>(); + + for (int t = 0; t < threads; t++) { + futures.add(pool.submit(() -> { + start.await(); + long count = 0; + while (System.nanoTime() < deadline) { + for (int i = 0; i < 100; i++) { + op.call(); + } + count += 100; + } + return count; + })); + } + + start.countDown(); + long total = 0; + for (Future future : futures) { + total += future.get(120, TimeUnit.SECONDS); + } + pool.shutdown(); + return total * 1000 / MEASURE_MILLIS; + } +} +``` + +Note the converter benchmark drives `convertValue` rather than the `protected getConverter` directly, so it exercises the real public path including the lock that Task 3 removed. + +- [ ] **Step 4: Capture before/after numbers** + +```bash +cd /Users/lukaszlenart/Projects/Apache/struts +git status --porcelain --untracked-files=no # must be empty + +# AFTER numbers, on the branch +mvn test -DskipAssembly -pl core -Dtest=Ww5539BenchmarkTest | tee /tmp/ww5539-after.txt + +# BEFORE numbers: stash the benchmark, check out main, reapply just the benchmark +git stash push --include-untracked core/src/test/java/org/apache/struts2/validator/Ww5539BenchmarkTest.java +git switch --detach main +git stash pop +mvn test -DskipAssembly -pl core -Dtest=Ww5539BenchmarkTest | tee /tmp/ww5539-before.txt + +# return to the branch +git stash push --include-untracked core/src/test/java/org/apache/struts2/validator/Ww5539BenchmarkTest.java +git switch WW-5539 +git stash pop +``` + +Record both sets in a markdown table: rows = thread counts, columns = before ops/sec, after ops/sec, ratio. + +Run each side twice and use the second run — the first is polluted by JIT and filesystem cache effects even with the warm-up loop. + +- [ ] **Step 5: Delete the benchmark** + +```bash +rm core/src/test/java/org/apache/struts2/validator/Ww5539BenchmarkTest.java +git status --porcelain # must show nothing to commit +``` + +Confirm it is gone before opening the PR. It must not appear in the diff. + +- [ ] **Step 6: Open the pull request** + +Report the benchmark results **faithfully**, including the realistic outcome that the converter changes may not move the needle while the validator lock does — the converter caches warm fast, and the validator lock is the coarser of the two. Do not present a null result as a win. + +The PR description must include: + +- `Fixes [WW-5539](https://issues.apache.org/jira/browse/WW-5539)` +- The before/after benchmark table with thread counts. +- A **Behaviour changes** section listing: + - `XWorkConverter.buildConverterMapping` (protected) no longer stores its result in the `TypeConverterHolder`. + - `StrutsTypeConverterHolder.unknownMappings` (protected) retyped from `HashSet` to `Set` and made `final` — a source-compatibility break for subclasses that assign it or call `HashSet`-specific methods. + - `addDefaultMapping` now ignores null converters with a warning instead of storing them. + - `XWorkConverter.registerConverter` / `registerConverterNotFound` are no longer `synchronized`. + - `TypeConverterHolder.getMapping`, `addMapping`, `containsNoMapping` deprecated. +- A **Known benign race** note: `lookup()` reads `containsUnknownMapping` and `containsDefaultMapping` as two separate atomic calls; the pair is not atomic, so a converter registered between them yields a stale `null` for that one call, self-correcting on the next lookup. No lock was added, because doing so would reintroduce the contention being removed on the hottest read path. +- A **Follow-ups** section: the classloader-pinning issue (`mappings` and `noMapping` hold strong `Class` references in a container singleton, keeping webapp classloaders alive across hot redeploy), and removal of the deprecated methods. + +Follow `~/.claude/pr_guideline.md`. + +--- + +## Notes for the implementer + +**Why `lookup()` keeps check-then-act.** It is tempting to "finish the job" and wrap the default-mappings cache in `computeIfAbsent` too. Do not. Its resolver, `lookupSuper()`, reads `getDefaultMapping()` recursively while walking the class hierarchy, and a recursive read inside `ConcurrentHashMap.computeIfAbsent` is explicitly forbidden — it deadlocks or throws `IllegalStateException: Recursive update`. The duplicated work on a cold miss is a handful of lock-free map reads. + +**Why `computeIfAbsent` on `mappings` is safe.** Its builder, `buildConverterMapping`, reaches `DefaultConversionFileProcessor` (which never touches the holder) and `DefaultConversionAnnotationProcessor` (which writes only to `defaultMappings`, a *different* map). Both were verified during design. If a future change makes the builder write to `mappings`, this becomes a deadlock — worth a comment if you touch that path. + +**Concurrency tests prove presence, not absence.** A green run on a strongly-ordered x86 machine is weak evidence. The primary correctness argument is the reasoning about the data structures; the tests are a backstop. Do not add sleeps or retry loops to make a flaky assertion pass — if one is flaky, the design reasoning is wrong and needs revisiting. + +**`conditionalReload` stays outside the compute** in `getConverter`. It only does work when `struts.configuration.xml.reload` is enabled and it must run on cache *hits* — that is its entire purpose. From 058d59a31d17863c8d8d9f169e4ba41cd7c8e77b Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Tue, 21 Jul 2026 16:54:30 +0200 Subject: [PATCH 03/20] WW-5539 docs: make conditionalReload store its rebuilt mapping buildConverterMapping no longer stores its result, so reload mode would have rebuilt from disk on every request without ever caching. --- ...6-07-21-WW-5539-concurrency-performance.md | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/docs/superpowers/plans/2026-07-21-WW-5539-concurrency-performance.md b/docs/superpowers/plans/2026-07-21-WW-5539-concurrency-performance.md index 1637ddd628..24ef23a4ef 100644 --- a/docs/superpowers/plans/2026-07-21-WW-5539-concurrency-performance.md +++ b/docs/superpowers/plans/2026-07-21-WW-5539-concurrency-performance.md @@ -727,6 +727,27 @@ That is, delete this block entirely: } ``` +Now make `conditionalReload` store the rebuilt mapping, which `buildConverterMapping` no longer does. Without this, reload mode returns fresh data but never caches it, rebuilding from disk on every single request. Replace `conditionalReload` (lines 580-591) with: + +```java + @SuppressWarnings("deprecation") + private Map conditionalReload(Class clazz, Map oldValues) throws Exception { + Map mapping = oldValues; + + if (reloadingConfigs) { + 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); + } + } + + return mapping; + } +``` + Update the method's Javadoc to record the behaviour change, since it is `protected` and visible to subclasses: ```java @@ -1165,4 +1186,4 @@ Follow `~/.claude/pr_guideline.md`. **Concurrency tests prove presence, not absence.** A green run on a strongly-ordered x86 machine is weak evidence. The primary correctness argument is the reasoning about the data structures; the tests are a backstop. Do not add sleeps or retry loops to make a flaky assertion pass — if one is flaky, the design reasoning is wrong and needs revisiting. -**`conditionalReload` stays outside the compute** in `getConverter`. It only does work when `struts.configuration.xml.reload` is enabled and it must run on cache *hits* — that is its entire purpose. +**`conditionalReload` stays outside the compute** in `getConverter`. It only does work when `struts.configuration.xml.reload` is enabled and it must run on cache *hits* — that is its entire purpose. It stores its own rebuild via the deprecated `addMapping`, because `computeMappingIfAbsent` cannot express an unconditional overwrite. That deprecated call is deliberate and carries a comment saying so; do not "clean it up". From 47ce7a8d3805054a1df64dab30feb28ffbc0aba5 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Tue, 21 Jul 2026 16:57:05 +0200 Subject: [PATCH 04/20] 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 +++- .../StrutsTypeConverterHolderTest.java | 133 ++++++++++++++++++ 2 files changed, 151 insertions(+), 6 deletions(-) create mode 100644 core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java 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..09f5766bc1 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. * 
@@ -34,7 +39,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,23 +60,30 @@ 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. */ - private final HashSet noMapping = new HashSet<>(); // action + private final Set noMapping = ConcurrentHashMap.newKeySet(); // action /** * Record classes that doesn't have conversion mapping defined. * 
      * - String -> classname as String
      *
+ * + * @deprecated since 7.3.0, this field is an implementation detail and will be made private. */ - protected HashSet unknownMappings = new HashSet<>(); // non-action (eg. returned value) + @Deprecated + protected final Set 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 0000000000..6fc2eac803 --- /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> 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> 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(); + } +} From 43ddc13ad160adc6c24c82efff7531e57b2219e8 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Tue, 21 Jul 2026 17:01:56 +0200 Subject: [PATCH 05/20] WW-5539 Rename test to match what it actually covers The method exercised only the unknown-mapping cache, not noMapping. --- .../struts2/conversion/StrutsTypeConverterHolderTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 6fc2eac803..be931af8ac 100644 --- a/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java +++ b/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java @@ -76,7 +76,7 @@ public void testConcurrentDefaultMappingRegistrationLosesNothing() throws Except } } - public void testConcurrentNoMappingAndUnknownMappingRegistrationLosesNothing() throws Exception { + public void testConcurrentUnknownMappingRegistrationLosesNothing() throws Exception { StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder(); ExecutorService pool = Executors.newFixedThreadPool(THREADS); CountDownLatch start = new CountDownLatch(1); From ff24097447eca6f995b1d1ed80f97b434b6a475f Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Tue, 21 Jul 2026 17:06:54 +0200 Subject: [PATCH 06/20] WW-5539 Add TypeConverterHolder#computeMappingIfAbsent Adds an atomic build-once-and-cache operation so callers no longer need check-then-act around the class mapping cache, and deprecates the three primitives it subsumes: getMapping, addMapping and containsNoMapping. The method is a default method delegating to those primitives, so third-party TypeConverterHolder implementations keep working unchanged. --- .../conversion/StrutsTypeConverterHolder.java | 21 +++++ .../conversion/TypeConverterHolder.java | 46 ++++++++++ .../StrutsTypeConverterHolderTest.java | 92 +++++++++++++++++++ 3 files changed, 159 insertions(+) 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 09f5766bc1..247f2badc8 100644 --- a/core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java +++ b/core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java @@ -21,9 +21,11 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import java.util.Collections; import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Function; /** * Default implementation of {@link TypeConverterHolder} @@ -99,20 +101,39 @@ public TypeConverter getDefaultMapping(String className) { } @Override + @Deprecated public Map getMapping(Class clazz) { return mappings.get(clazz); } @Override + @Deprecated public void addMapping(Class clazz, Map mapping) { mappings.put(clazz, mapping); } @Override + @Deprecated public boolean containsNoMapping(Class clazz) { return noMapping.contains(clazz); } + @Override + public Map computeMappingIfAbsent(Class clazz, Function> builder) { + if (noMapping.contains(clazz)) { + return Collections.emptyMap(); + } + Map mapping = mappings.computeIfAbsent(clazz, c -> { + Map built = builder.apply(c); + return (built == null || built.isEmpty()) ? null : built; + }); + if (mapping == null) { + noMapping.add(clazz); + return Collections.emptyMap(); + } + return mapping; + } + @Override public void addNoMapping(Class clazz) { noMapping.add(clazz); 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..eb710e4089 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 @@ -54,7 +56,10 @@ public interface TypeConverterHolder { * * @param clazz class to convert to/from * @return {@link TypeConverter} for given class + * @deprecated since 7.3.0, use {@link #computeMappingIfAbsent(Class, Function)} which resolves + * and caches the mapping atomically instead of requiring a check-then-act at the call site. */ + @Deprecated Map getMapping(Class clazz); /** @@ -62,7 +67,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 void addMapping(Class clazz, Map mapping); /** @@ -70,7 +78,10 @@ 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 boolean containsNoMapping(Class clazz); /** @@ -97,4 +108,39 @@ 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 + * {@link Collections#emptyMap()}. + * + *

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 are expected to make this atomic so that the builder runs at most once per + * class. 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/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java b/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java index be931af8ac..efb7f0fc12 100644 --- a/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java +++ b/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java @@ -21,12 +21,16 @@ import org.apache.struts2.XWorkTestCase; import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; 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 java.util.concurrent.atomic.AtomicInteger; import static org.assertj.core.api.Assertions.assertThat; @@ -130,4 +134,92 @@ public void testAddDefaultMappingClearsUnknownMapping() { assertThat(holder.containsUnknownMapping("stub.Later")).isFalse(); assertThat(holder.getDefaultMapping("stub.Later")).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(); + } + + 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(); + } + + public void testComputeMappingIfAbsentBuildsOnceUnderConcurrency() throws Exception { + StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder(); + AtomicInteger builds = new AtomicInteger(); + 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 -> { + builds.incrementAndGet(); + 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(builds.get()).as("mapping must be built exactly once").isEqualTo(1); + } } From 1bc7b33d168f9d15271a91a9eb7b3698ea2ef52b Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Tue, 21 Jul 2026 17:18:15 +0200 Subject: [PATCH 07/20] 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 +++++++++---------- .../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 247f2badc8..4b53b1dda0 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.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> 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 noMapping = ConcurrentHashMap.newKeySet(); // action + private static final Map NO_MAPPING = Collections.unmodifiableMap(new HashMap<>()); /** * Record classes that doesn't have conversion mapping defined. @@ -103,7 +107,8 @@ public TypeConverter getDefaultMapping(String className) { @Override @Deprecated public Map getMapping(Class clazz) { - return mappings.get(clazz); + Map mapping = mappings.get(clazz); + return mapping == NO_MAPPING ? null : mapping; } @Override @@ -115,28 +120,20 @@ public void addMapping(Class clazz, Map mapping) { @Override @Deprecated public boolean containsNoMapping(Class clazz) { - return noMapping.contains(clazz); + return mappings.get(clazz) == NO_MAPPING; } @Override - public Map computeMappingIfAbsent(Class clazz, Function> builder) { - if (noMapping.contains(clazz)) { - return Collections.emptyMap(); - } - Map mapping = mappings.computeIfAbsent(clazz, c -> { - Map 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 computeMappingIfAbsent(Class clazz, Function> builder) { + return mappings.computeIfAbsent(clazz, c -> { + Map 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 efb7f0fc12..34dc18ad0a 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 void testComputeMappingIfAbsentBuildsOnceUnderConcurrency() throws Except 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>> 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> 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(); + } } From 0e99d8a2b76285a5d041695a904d4ae552bb38b9 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Tue, 21 Jul 2026 17:19:20 +0200 Subject: [PATCH 08/20] WW-5539 docs: sync plan with negative-cache sentinel fix --- ...6-07-21-WW-5539-concurrency-performance.md | 42 ++++++++++++++----- 1 file changed, 31 insertions(+), 11 deletions(-) diff --git a/docs/superpowers/plans/2026-07-21-WW-5539-concurrency-performance.md b/docs/superpowers/plans/2026-07-21-WW-5539-concurrency-performance.md index 24ef23a4ef..4858b474d2 100644 --- a/docs/superpowers/plans/2026-07-21-WW-5539-concurrency-performance.md +++ b/docs/superpowers/plans/2026-07-21-WW-5539-concurrency-performance.md @@ -503,25 +503,45 @@ import java.util.function.Function; Add the override after `getMapping`, and mark the three implementing methods `@Deprecated` to match the interface: +Negative results are stored as a sentinel in the same map, so `computeIfAbsent` deduplicates the builder on **both** paths. This matters because the no-mapping case is the *common* one — an ordinary action class has no `-conversion.properties` and no `@Conversion` annotations, yet `buildConverterMapping` still walks its whole hierarchy doing classpath lookups and reflection. Returning `null` from the mapping function would store nothing, letting every concurrent caller re-run that walk: the exact thundering herd this method exists to prevent. + +Add the sentinel. It MUST be a distinct instance, not `Collections.emptyMap()`, whose shared JDK singleton could collide with an empty map a caller passed to `addMapping`: + +```java + private static final Map NO_MAPPING = Collections.unmodifiableMap(new HashMap<>()); +``` + +Delete the `noMapping` field — the sentinel replaces it. Then: + ```java + @Override + @Deprecated + public Map getMapping(Class clazz) { + Map mapping = mappings.get(clazz); + return mapping == NO_MAPPING ? null : mapping; + } + + @Override + @Deprecated + public boolean containsNoMapping(Class clazz) { + return mappings.get(clazz) == NO_MAPPING; + } + + @Override + public void addNoMapping(Class clazz) { + mappings.put(clazz, NO_MAPPING); + } + @Override public Map computeMappingIfAbsent(Class clazz, Function> builder) { - if (noMapping.contains(clazz)) { - return Collections.emptyMap(); - } - Map mapping = mappings.computeIfAbsent(clazz, c -> { + return mappings.computeIfAbsent(clazz, c -> { Map built = builder.apply(c); - return (built == null || built.isEmpty()) ? null : built; + return (built == null || built.isEmpty()) ? NO_MAPPING : built; }); - if (mapping == null) { - noMapping.add(clazz); - return Collections.emptyMap(); - } - return mapping; } ``` -Returning `null` from a `computeIfAbsent` mapping function stores nothing and yields `null`, so the negative case falls out naturally and `getMapping()` keeps its "null when absent" meaning. +`getMapping` and `containsNoMapping` translate the sentinel, so both keep their existing contracts — `getMapping` still returns `null` for an absent *or* negative-cached class. Add `@Deprecated` to the `getMapping`, `addMapping` and `containsNoMapping` overrides in this class so they do not emit "overrides deprecated method" noise. From ebf341aeeb991035a04507a7bb2ce131076e67ea Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Tue, 21 Jul 2026 17:27:37 +0200 Subject: [PATCH 09/20] WW-5539 Pin down addNoMapping's override semantics Storing the no-mapping sentinel deliberately replaces any mapping cached for the class, matching the pre-7.3.0 effective behaviour where such a class was short-circuited before its cached mapping was ever read. putIfAbsent would instead serve a stale mapping after a failed build. Also asserts the sentinel translation in getMapping directly, and stops the interface javadoc promising a specific empty-map instance that implementations are not required to return. --- .../conversion/StrutsTypeConverterHolder.java | 10 ++++++++++ .../struts2/conversion/TypeConverterHolder.java | 3 +-- .../conversion/StrutsTypeConverterHolderTest.java | 14 ++++++++++++++ 3 files changed, 25 insertions(+), 2 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 4b53b1dda0..4a2bed820f 100644 --- a/core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java +++ b/core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java @@ -123,6 +123,16 @@ public boolean containsNoMapping(Class 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) { mappings.put(clazz, NO_MAPPING); 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 eb710e4089..1adfe8a3cc 100644 --- a/core/src/main/java/org/apache/struts2/conversion/TypeConverterHolder.java +++ b/core/src/main/java/org/apache/struts2/conversion/TypeConverterHolder.java @@ -110,8 +110,7 @@ public interface TypeConverterHolder { /** * 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 - * {@link Collections#emptyMap()}. + * 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.

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 34dc18ad0a..686ab5f4da 100644 --- a/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java +++ b/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java @@ -172,6 +172,20 @@ public void testComputeMappingIfAbsentNegativeCachesEmptyResult() { 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(); + } + + 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 -> real)).isEmpty(); } public void testComputeMappingIfAbsentNegativeCachesNullResult() { From f64a19006a3603f0c4c474f2a7b1127e660d3674 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Tue, 21 Jul 2026 17:34:45 +0200 Subject: [PATCH 10/20] WW-5539 Document that addNoMapping may replace a cached mapping The behaviour was documented only on the Struts implementation, but addNoMapping stays a non-deprecated SPI primitive that third parties both call and implement, so the contract belongs on the interface. Co-Authored-By: Claude Opus 4.8 --- .../org/apache/struts2/conversion/TypeConverterHolder.java | 4 +++- .../struts2/conversion/StrutsTypeConverterHolderTest.java | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) 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 1adfe8a3cc..ccfa7d1e1c 100644 --- a/core/src/main/java/org/apache/struts2/conversion/TypeConverterHolder.java +++ b/core/src/main/java/org/apache/struts2/conversion/TypeConverterHolder.java @@ -85,7 +85,9 @@ public interface TypeConverterHolder { 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 */ 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 686ab5f4da..b31ff4f93c 100644 --- a/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java +++ b/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java @@ -185,7 +185,9 @@ public void testAddNoMappingOverridesPreviouslyCachedMapping() { assertThat(holder.containsNoMapping(String.class)).isTrue(); assertThat(holder.getMapping(String.class)).isNull(); - assertThat(holder.computeMappingIfAbsent(String.class, clazz -> real)).isEmpty(); + assertThat(holder.computeMappingIfAbsent(String.class, clazz -> { + throw new AssertionError("builder must not run when no mapping is set"); + })).isNotNull().isEmpty(); } public void testComputeMappingIfAbsentNegativeCachesNullResult() { From c9e6c17ea0b8eb831c6e908374c1d3830e9a26c5 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Tue, 21 Jul 2026 17:38:48 +0200 Subject: [PATCH 11/20] 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. --- .../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 89d3ecceb4..58a075efdd 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 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); - } - - 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); + if (property == null) { + return null; + } + try { + Map 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 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 +470,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 +554,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 +577,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") private Map conditionalReload(Class clazz, Map oldValues) throws Exception { Map mapping = oldValues; @@ -584,6 +588,9 @@ private Map conditionalReload(Class clazz, Map o 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 39fe8e2cf5..14aeb39054 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.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.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 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() 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> 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 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(); From f1a29d560ffd1d3d2be49954449481b61196028d Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Tue, 21 Jul 2026 17:51:41 +0200 Subject: [PATCH 12/20] 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. --- .../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 2f2b758b79..530b89599a 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); } @@ -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/test/java/org/apache/struts2/validator/DefaultActionValidatorManagerTest.java b/core/src/test/java/org/apache/struts2/validator/DefaultActionValidatorManagerTest.java index 06914d31f5..041ca76473 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.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.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 void testFieldErrorsOrder() throws Exception { 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> 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 future : futures) { + assertThat(future.get(60, TimeUnit.SECONDS)).isEqualTo(expectedCount); + } + pool.shutdown(); + } + + public void testCachedValidatorConfigsAreUnmodifiable() { + actionValidatorManager.getValidators(SimpleAction.class, alias); + + List cached = actionValidatorManager.validatorCache.values().iterator().next(); + + assertThatThrownBy(() -> cached.add(null)) + .isInstanceOf(UnsupportedOperationException.class); + } + } From 47e992d2005bd2ac287ada7a3c7407d6d1039328 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Tue, 21 Jul 2026 17:58:36 +0200 Subject: [PATCH 13/20] WW-5539 Make the validator concurrency test race a cold cache The test computed its expected count with a getValidators call before starting the threads, which warmed the cache and left all 16 workers on the fast path - never exercising first-build contention, the race the test is named for. Also drops an unused import and awaits executor termination. --- .../struts2/conversion/impl/XWorkConverterTest.java | 1 - .../validator/DefaultActionValidatorManagerTest.java | 9 +++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) 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 14aeb39054..74e8176bd2 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,7 +37,6 @@ 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; 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 041ca76473..af388c4958 100644 --- a/core/src/test/java/org/apache/struts2/validator/DefaultActionValidatorManagerTest.java +++ b/core/src/test/java/org/apache/struts2/validator/DefaultActionValidatorManagerTest.java @@ -388,7 +388,9 @@ public void testConcurrentGetValidatorsReturnsConsistentResults() throws Excepti CountDownLatch start = new CountDownLatch(1); List> futures = new ArrayList<>(); - int expectedCount = actionValidatorManager.getValidators(SimpleAction.class, alias).size(); + // actionValidatorManager is freshly injected in setUp(), so validatorCache is cold here - + // all 16 threads below race the first build for this key, which is the interesting + // contention this test exists to cover. // 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. @@ -404,10 +406,13 @@ public void testConcurrentGetValidatorsReturnsConsistentResults() throws Excepti } start.countDown(); + int firstSize = futures.get(0).get(60, TimeUnit.SECONDS); + assertThat(firstSize).isGreaterThan(0); for (Future future : futures) { - assertThat(future.get(60, TimeUnit.SECONDS)).isEqualTo(expectedCount); + assertThat(future.get(60, TimeUnit.SECONDS)).isEqualTo(firstSize); } pool.shutdown(); + assertThat(pool.awaitTermination(60, TimeUnit.SECONDS)).isTrue(); } public void testCachedValidatorConfigsAreUnmodifiable() { From 4de29a145f703fdde2bb81e867ce29af41142e5d Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Tue, 21 Jul 2026 20:45:38 +0200 Subject: [PATCH 14/20] WW-5539 Address final review findings Restores the protected unknownMappings field verbatim as a deprecated, unused vestige: retyping it changed the field descriptor, so a subclass compiled against 7.2.0 would have hit NoSuchFieldError on upgrade without recompiling. Real storage moves to a private concurrent set. Also stops conditionalReload running for negative-cached classes, which had been costing a failed classloader resource scan per property per request in devMode, and restores the unknown-mapping clearing that the null-converter guard was skipping. --- .../conversion/StrutsTypeConverterHolder.java | 19 ++++++++++++++----- .../conversion/TypeConverterHolder.java | 4 ++++ .../conversion/impl/XWorkConverter.java | 4 +++- .../DefaultActionValidatorManager.java | 2 +- 4 files changed, 22 insertions(+), 7 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 4a2bed820f..e6ec37b47e 100644 --- a/core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java +++ b/core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java @@ -23,6 +23,7 @@ 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; @@ -79,19 +80,27 @@ public class StrutsTypeConverterHolder implements TypeConverterHolder { * - String -> classname as String * * - * @deprecated since 7.3.0, this field is an implementation detail and will be made private. + * @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 - protected final Set unknownMappings = ConcurrentHashMap.newKeySet(); // non-action (eg. returned value) + 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. + */ + private final Set unknownMappingsInternal = ConcurrentHashMap.newKeySet(); @Override public void addDefaultMapping(String className, TypeConverter typeConverter) { + unknownMappingsInternal.remove(className); if (typeConverter == null) { LOG.warn("Ignoring null TypeConverter registered for class [{}]", className); return; } defaultMappings.put(className, typeConverter); - unknownMappings.remove(className); } @Override @@ -148,12 +157,12 @@ public Map computeMappingIfAbsent(Class clazz, FunctionReturns {@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 * @deprecated since 7.3.0, use {@link #computeMappingIfAbsent(Class, Function)} which resolves 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 58a075efdd..419e8b0057 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 @@ -419,7 +419,9 @@ protected Object getConverter(Class clazz, String property) { } try { Map mapping = converterHolder.computeMappingIfAbsent(clazz, this::buildConverterMappingUnchecked); - mapping = conditionalReload(clazz, mapping); + if (!mapping.isEmpty()) { + mapping = conditionalReload(clazz, mapping); + } Object converter = mapping.get(property); if (converter == null && LOG.isDebugEnabled()) { 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 530b89599a..7da4bccf86 100644 --- a/core/src/main/java/org/apache/struts2/validator/DefaultActionValidatorManager.java +++ b/core/src/main/java/org/apache/struts2/validator/DefaultActionValidatorManager.java @@ -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()) { From 82045012b425d8c199a02efefb82d8f7fce37556 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Tue, 21 Jul 2026 21:25:46 +0200 Subject: [PATCH 15/20] WW-5539 Fix concurrency regressions from coarse-lock removal Four correctness fixes surfaced in PR review of the concurrent-collections refactor: - StrutsTypeConverterHolder.addDefaultMapping: restore put-before-remove ordering. The inverted order let a concurrent XWorkConverter.lookup observe (unknown=false, default=false), sending it into lookupSuper() and letting it overwrite the more specific converter being registered. - StrutsTypeConverterHolder.computeMappingIfAbsent: stop building inside a ConcurrentHashMap bin lock. The builder reaches ObjectFactory.buildConverter, which can autowire arbitrary user TypeConverters; running that under a CHM bin lock risked a recursive-update exception or self-deadlock. Callers now only get the guarantee that they converge on the same cached instance, not that the builder runs exactly once - documented on the interface and reflected in the concurrency tests. - DefaultValidatorFactory.validators: switch to ConcurrentHashMap now that DefaultActionValidatorManager.getValidators is no longer synchronized, so runtime registerValidator() calls no longer race unsynchronized reads of a plain HashMap. - XWorkConverter.conditionalReload: route empty devMode rebuilds through addNoMapping instead of addMapping, so an empty reload result is stored as the NO_MAPPING sentinel rather than a plain empty map that would silently disable further reloads for the class. Co-Authored-By: Claude Opus 4.8 --- .../conversion/StrutsTypeConverterHolder.java | 26 ++- .../conversion/TypeConverterHolder.java | 10 +- .../conversion/impl/XWorkConverter.java | 10 +- .../validator/DefaultValidatorFactory.java | 4 +- .../StrutsTypeConverterHolderTest.java | 182 ++++++++++++++++-- .../conversion/impl/XWorkConverterTest.java | 10 +- 6 files changed, 214 insertions(+), 28 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 e6ec37b47e..95a8097831 100644 --- a/core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java +++ b/core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java @@ -95,12 +95,16 @@ public class StrutsTypeConverterHolder implements TypeConverterHolder { @Override public void addDefaultMapping(String className, TypeConverter typeConverter) { - unknownMappingsInternal.remove(className); 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); + unknownMappingsInternal.remove(className); } @Override @@ -149,10 +153,22 @@ public void addNoMapping(Class clazz) { @Override public Map computeMappingIfAbsent(Class clazz, Function> builder) { - return mappings.computeIfAbsent(clazz, c -> { - Map built = builder.apply(c); - return (built == null || built.isEmpty()) ? NO_MAPPING : built; - }); + // 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 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 7518fc30d7..745595b992 100644 --- a/core/src/main/java/org/apache/struts2/conversion/TypeConverterHolder.java +++ b/core/src/main/java/org/apache/struts2/conversion/TypeConverterHolder.java @@ -121,9 +121,13 @@ public interface TypeConverterHolder { *

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 are expected to make this atomic so that the builder runs at most once per - * class. 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.

+ *

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 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 419e8b0057..3da17a7a4a 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 @@ -590,9 +590,13 @@ private Map conditionalReload(Class clazz, Map o 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); + 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/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 index b31ff4f93c..9982561a5c 100644 --- a/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java +++ b/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java @@ -20,11 +20,16 @@ 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; @@ -135,6 +140,155 @@ public void testAddDefaultMappingClearsUnknownMapping() { 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(); @@ -210,9 +364,14 @@ public void testComputeMappingIfAbsentShortCircuitsOnKnownNoMapping() { assertThat(result).isNotNull().isEmpty(); } - public void testComputeMappingIfAbsentBuildsOnceUnderConcurrency() throws Exception { + /** + * {@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(); - AtomicInteger builds = new AtomicInteger(); ExecutorService pool = Executors.newFixedThreadPool(THREADS); CountDownLatch start = new CountDownLatch(1); List>> futures = new ArrayList<>(); @@ -221,7 +380,6 @@ public void testComputeMappingIfAbsentBuildsOnceUnderConcurrency() throws Except futures.add(pool.submit(() -> { start.await(); return holder.computeMappingIfAbsent(String.class, clazz -> { - builds.incrementAndGet(); Map built = new HashMap<>(); built.put("someProperty", "someConverter"); return built; @@ -236,12 +394,15 @@ public void testComputeMappingIfAbsentBuildsOnceUnderConcurrency() throws Except } pool.shutdown(); - assertThat(builds.get()).as("mapping must be built exactly once").isEqualTo(1); + assertThat(expected).containsEntry("someProperty", "someConverter"); } - public void testComputeMappingIfAbsentBuildsOnceUnderConcurrencyForUnmappedClass() throws Exception { + /** + * 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(); - AtomicInteger builds = new AtomicInteger(); ExecutorService pool = Executors.newFixedThreadPool(THREADS); CountDownLatch start = new CountDownLatch(1); List>> futures = new ArrayList<>(); @@ -249,20 +410,17 @@ public void testComputeMappingIfAbsentBuildsOnceUnderConcurrencyForUnmappedClass for (int t = 0; t < THREADS; t++) { futures.add(pool.submit(() -> { start.await(); - return holder.computeMappingIfAbsent(String.class, clazz -> { - builds.incrementAndGet(); - return Collections.emptyMap(); - }); + 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)).isEmpty(); + assertThat(future.get(60, TimeUnit.SECONDS)).isSameAs(expected).isEmpty(); } pool.shutdown(); - assertThat(builds.get()).as("unmapped class must be built exactly once").isEqualTo(1); assertThat(holder.containsNoMapping(String.class)).isTrue(); } } 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 74e8176bd2..7fed13b941 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 @@ -838,7 +838,13 @@ public void testGetConverterBuildsMappingExactlyOncePerClass() throws Exception assertEquals(1, countingConverter.builds.get()); } - public void testGetConverterBuildsMappingExactlyOnceUnderConcurrency() throws Exception { + /** + * {@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()); @@ -859,8 +865,6 @@ public void testGetConverterBuildsMappingExactlyOnceUnderConcurrency() throws Ex assertEquals(String.class, future.get(60, TimeUnit.SECONDS)); } pool.shutdown(); - - assertEquals(1, countingConverter.builds.get()); } public void testGetConverterReturnsNullForUnknownProperty() { From e4c28df1d1c85c43ec7e46b5e45baec3f99392a3 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Tue, 21 Jul 2026 21:43:19 +0200 Subject: [PATCH 16/20] WW-5539 Add tests closing coverage gaps from coarse-lock removal SonarCloud's quality gate failed at 59.8% coverage on new code (need >=80%). Adds tests for the specific lines JaCoCo identified as uncovered, without touching production code: - TypeConverterHolder.computeMappingIfAbsent's default method body (the SPI compatibility fallback for third-party holders that predate 7.3.0 and don't override it) - new TypeConverterHolderTest against a minimal non-overriding implementation. - StrutsTypeConverterHolder.getMapping/containsNoMapping's remaining non-sentinel branch. - XWorkConverter.conditionalReload's reloadingConfigs==true path (both the addMapping and addNoMapping outcomes), buildConverterMappingUnchecked's checked-to-IllegalStateException wrapping, and getConverter's catch(Throwable) negative-caching. - DefaultActionValidatorManager's else-if(reloadingConfigs) cache rebuild, loadFile's checkFile&&fileNeedsReloading re-parse, and buildValidatorConfigs' already-checked short-circuit. TypeConverterHolder.java and StrutsTypeConverterHolder.java are now at 0 missed lines/branches. XWorkConverter.java and DefaultActionValidatorManager.java have all requested target lines covered; remaining misses are pre-existing, unrelated gaps left alone per scope. Full core suite: 3026 tests (3015 + 11 new), 0 failures/errors. Co-Authored-By: Claude Opus 4.8 --- .../StrutsTypeConverterHolderTest.java | 16 ++ .../conversion/TypeConverterHolderTest.java | 151 ++++++++++++++++ .../conversion/impl/XWorkConverterTest.java | 171 ++++++++++++++++++ .../DefaultActionValidatorManagerTest.java | 133 ++++++++++++++ 4 files changed, 471 insertions(+) create mode 100644 core/src/test/java/org/apache/struts2/conversion/TypeConverterHolderTest.java 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 9982561a5c..df28e2dfa9 100644 --- a/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java +++ b/core/src/test/java/org/apache/struts2/conversion/StrutsTypeConverterHolderTest.java @@ -329,6 +329,22 @@ public void testComputeMappingIfAbsentNegativeCachesEmptyResult() { 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<>(); 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 7fed13b941..52d90df5fc 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; @@ -40,6 +41,8 @@ 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; @@ -881,6 +884,174 @@ public void testGetConverterReturnsNullForNullProperty() { assertNull(freshConverter.getConverter(User.class, null)); } + /** + * Minimal {@link FileManager} whose {@code fileNeedsReloading} answer is fixed at construction, + * so {@code conditionalReload}'s branches can be driven deterministically without depending on + * real file timestamps or the shared, statically-cached {@link org.apache.struts2.util.fs.DefaultFileManager}. + * {@code loadFile} is never exercised through this seam: {@code XWorkConverter.fileManager} is + * only ever read by {@code conditionalReload}'s {@code fileNeedsReloading} call - actual property + * file parsing goes through {@link org.apache.struts2.conversion.impl.DefaultConversionFileProcessor}'s + * own, separately-injected {@link FileManager}. + */ + private static class StubFileManager implements FileManager { + private final boolean needsReloading; + + StubFileManager(boolean needsReloading) { + this.needsReloading = needsReloading; + } + + @Override + public void setReloadingConfigs(boolean reloadingConfigs) { + } + + @Override + public boolean fileNeedsReloading(String fileName) { + return needsReloading; + } + + @Override + public boolean fileNeedsReloading(URL fileUrl) { + return needsReloading; + } + + @Override + public InputStream loadFile(URL fileUrl) { + throw new UnsupportedOperationException("not exercised via XWorkConverter.fileManager"); + } + + @Override + public void monitorFile(URL fileUrl) { + } + + @Override + public URL normalizeToFileProtocol(URL url) { + return url; + } + + @Override + public boolean support() { + return true; + } + + @Override + public boolean internal() { + return true; + } + + @Override + public Collection getAllPhysicalUrls(URL url) { + return List.of(url); + } + } + + private static void setFileManager(XWorkConverter converter, FileManager fileManager) throws Exception { + Field field = XWorkConverter.class.getDeclaredField("fileManager"); + field.setAccessible(true); + field.set(converter, fileManager); + } + + /** + * Covers {@code conditionalReload}'s {@code reloadingConfigs == true} branch where the rebuilt + * mapping is real (non-empty), so it is stored via {@code addMapping}. {@code User} has a real + * {@code User-conversion.properties} on the test classpath (see {@code Collection_list} used + * elsewhere in this file), so {@code buildConverterMapping} legitimately returns a non-empty + * mapping both for the initial cache population and for the forced reload. + */ + public void testConditionalReloadRebuildsRealMappingAndStoresItViaAddMapping() throws Exception { + CountingXWorkConverter countingConverter = container.inject(CountingXWorkConverter.class); + StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder(); + countingConverter.setTypeConverterHolder(holder); + countingConverter.setReloadingConfigs("true"); + setFileManager(countingConverter, new StubFileManager(true)); + + Object converterForList = countingConverter.getConverter(User.class, "Collection_list"); + + assertEquals(String.class, converterForList); + assertEquals("buildConverterMapping must run once for the initial cache miss and once more " + + "for the forced reload", + 2, countingConverter.builds.get()); + assertFalse("the reload found a real mapping, so the class must not be negative-cached", + holder.containsNoMapping(User.class)); + } + + /** + * Toggles between a real and an empty mapping across successive {@code buildConverterMapping} + * calls, so the forced reload in {@code conditionalReload} can be made to observe an empty + * rebuild independently of any real property file's contents. + */ + public static class ToggleXWorkConverter extends XWorkConverter { + final AtomicInteger calls = new AtomicInteger(); + + public ToggleXWorkConverter() { + super(); + } + + @Override + protected Map buildConverterMapping(Class clazz) { + if (calls.incrementAndGet() == 1) { + Map real = new HashMap<>(); + real.put("someProperty", "someConverter"); + return real; + } + return Collections.emptyMap(); + } + } + + /** + * Covers {@code conditionalReload}'s {@code reloadingConfigs == true} branch where the rebuilt + * mapping is empty, so it is routed to {@code addNoMapping} instead of {@code addMapping}. + */ + public void testConditionalReloadRebuildsEmptyMappingAndStoresItViaAddNoMapping() throws Exception { + ToggleXWorkConverter toggleConverter = container.inject(ToggleXWorkConverter.class); + StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder(); + toggleConverter.setTypeConverterHolder(holder); + toggleConverter.setReloadingConfigs("true"); + setFileManager(toggleConverter, new StubFileManager(true)); + + Object converter = toggleConverter.getConverter(User.class, "someProperty"); + + assertNull("the reload rebuilt an empty mapping, so no converter can be found", converter); + assertEquals("buildConverterMapping must run once for the initial cache miss (real mapping) " + + "and once more for the forced reload (empty mapping)", + 2, toggleConverter.calls.get()); + assertTrue("an empty rebuild must negative-cache the class", holder.containsNoMapping(User.class)); + } + + /** + * A {@code buildConverterMapping} that always fails with a checked exception, to drive + * {@code buildConverterMappingUnchecked}'s wrapping of it as an unchecked + * {@code IllegalStateException}, and {@code getConverter}'s {@code catch (Throwable)} + * negative-caching of the failure. + */ + public static class ThrowingXWorkConverter extends XWorkConverter { + final AtomicInteger attempts = new AtomicInteger(); + + public ThrowingXWorkConverter() { + super(); + } + + @Override + protected Map buildConverterMapping(Class clazz) throws Exception { + attempts.incrementAndGet(); + throw new Exception("simulated checked failure building converter mapping for " + clazz); + } + } + + public void testGetConverterNegativeCachesOnBuildFailure() { + ThrowingXWorkConverter throwingConverter = container.inject(ThrowingXWorkConverter.class); + StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder(); + throwingConverter.setTypeConverterHolder(holder); + + Object first = throwingConverter.getConverter(User.class, "Collection_list"); + assertNull(first); + assertTrue("a build failure must negative-cache the class", holder.containsNoMapping(User.class)); + + Object second = throwingConverter.getConverter(User.class, "Collection_list"); + assertNull(second); + assertEquals("the negative cache must prevent a second build attempt", + 1, throwingConverter.attempts.get()); + } + public static class Foo1 { public Bar1 getBar() { return new Bar1Impl(); 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 af388c4958..4cbf05c3c7 100644 --- a/core/src/test/java/org/apache/struts2/validator/DefaultActionValidatorManagerTest.java +++ b/core/src/test/java/org/apache/struts2/validator/DefaultActionValidatorManagerTest.java @@ -18,6 +18,7 @@ */ package org.apache.struts2.validator; +import org.apache.struts2.FileManager; import org.apache.struts2.FileManagerFactory; import org.apache.struts2.SimpleAction; import org.apache.struts2.TestBean; @@ -41,10 +42,16 @@ import org.assertj.core.api.Assertions; import org.xml.sax.SAXParseException; +import java.io.IOException; +import java.io.InputStream; +import java.net.URL; import java.util.ArrayList; +import java.util.Collection; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Set; +import java.util.TreeSet; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -424,4 +431,130 @@ public void testCachedValidatorConfigsAreUnmodifiable() { .isInstanceOf(UnsupportedOperationException.class); } + /** + * Covers {@code getValidators}' {@code else if (reloadingConfigs)} branch: reached when the + * validator-config key is already cached and reload mode is on, in which case the + * configs are rebuilt and the cache entry is replaced rather than reused. Rebuilding produces a + * fresh {@link List} instance even though the content is unchanged, so object identity is the + * real, observable signal that this branch - rather than the {@code computeIfAbsent} branch - + * ran. + */ + public void testGetValidatorsRebuildsCacheWhenReloadingConfigsEnabled() { + actionValidatorManager.getValidators(SimpleAction.class, alias); + String key = actionValidatorManager.buildValidatorKey(SimpleAction.class, alias); + List cachedBeforeReload = actionValidatorManager.validatorCache.get(key); + assertThat(cachedBeforeReload).isNotNull().isNotEmpty(); + + actionValidatorManager.reloadingConfigs = true; + actionValidatorManager.getValidators(SimpleAction.class, alias); + + List cachedAfterReload = actionValidatorManager.validatorCache.get(key); + assertThat(cachedAfterReload) + .as("the else-if(reloadingConfigs) branch must rebuild and replace the cache entry, " + + "not reuse the previously cached list") + .isNotSameAs(cachedBeforeReload); + assertThat(cachedAfterReload).hasSameSizeAs(cachedBeforeReload); + } + + /** + * Minimal {@link FileManager} wrapping a real one, whose {@code fileNeedsReloading} answer is + * fixed at construction. {@code loadFile} and everything else delegate to the real instance so + * that the actual validation XML on the test classpath is genuinely parsed - only the reload + * check itself is made deterministic. + */ + private static class ReloadFlagFileManager implements FileManager { + private final FileManager delegate; + private final boolean needsReloading; + + ReloadFlagFileManager(FileManager delegate, boolean needsReloading) { + this.delegate = delegate; + this.needsReloading = needsReloading; + } + + @Override + public void setReloadingConfigs(boolean reloadingConfigs) { + delegate.setReloadingConfigs(reloadingConfigs); + } + + @Override + public boolean fileNeedsReloading(String fileName) { + return needsReloading; + } + + @Override + public boolean fileNeedsReloading(URL fileUrl) { + return needsReloading; + } + + @Override + public InputStream loadFile(URL fileUrl) { + return delegate.loadFile(fileUrl); + } + + @Override + public void monitorFile(URL fileUrl) { + delegate.monitorFile(fileUrl); + } + + @Override + public URL normalizeToFileProtocol(URL url) { + return delegate.normalizeToFileProtocol(url); + } + + @Override + public boolean support() { + return delegate.support(); + } + + @Override + public boolean internal() { + return delegate.internal(); + } + + @Override + public Collection getAllPhysicalUrls(URL url) throws IOException { + return delegate.getAllPhysicalUrls(url); + } + } + + /** + * Covers {@code loadFile}'s {@code checkFile && fileManager.fileNeedsReloading(...)} branch, + * which re-parses and {@code put}s directly rather than using {@code computeIfAbsent}. Forcing + * a real re-parse produces a new {@link List} instance even though {@code SimpleAction- + * validationAlias-validation.xml} hasn't changed, so object identity is the observable proof + * that this branch - rather than the {@code computeIfAbsent} fallback - ran. + */ + public void testLoadFileReparsesWhenCheckFileAndFileNeedsReloading() { + String fileName = SimpleAction.class.getName().replace('.', '/') + "-" + alias + "-validation.xml"; + FileManager realFileManager = actionValidatorManager.fileManager; + + actionValidatorManager.fileManager = new ReloadFlagFileManager(realFileManager, false); + List warm = actionValidatorManager.loadFile(fileName, SimpleAction.class, false); + assertThat(warm).isNotEmpty(); + assertThat(actionValidatorManager.validatorFileCache.get(fileName)).isSameAs(warm); + + actionValidatorManager.fileManager = new ReloadFlagFileManager(realFileManager, true); + List reloaded = actionValidatorManager.loadFile(fileName, SimpleAction.class, true); + + assertThat(reloaded) + .as("checkFile && fileNeedsReloading must re-parse rather than reuse the cached list") + .isNotSameAs(warm); + assertThat(reloaded).hasSameSizeAs(warm); + assertThat(actionValidatorManager.validatorFileCache.get(fileName)).isSameAs(reloaded); + } + + /** + * Covers {@code buildValidatorConfigs} returning {@code Collections.emptyList()} when the class + * was already recorded in the {@code checked} set - the short-circuit that stops the hierarchy + * walk from revisiting a class (e.g. an interface reachable through more than one path). + */ + public void testBuildValidatorConfigsShortCircuitsWhenClassAlreadyChecked() { + Set checked = new TreeSet<>(); + checked.add(SimpleAction.class.getName()); + + List result = actionValidatorManager.buildValidatorConfigs(SimpleAction.class, alias, false, checked); + + assertThat(result).isNotNull().isEmpty(); + } + } From 7e7d4892fcb94ba413d153aeeed21b8ffe6e9b00 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Tue, 21 Jul 2026 22:06:31 +0200 Subject: [PATCH 17/20] WW-5539 Fix SonarCloud deprecation and test-hygiene issues - Add since/forRemoval attributes to the 7 @Deprecated elements on TypeConverterHolder.getMapping/addMapping/containsNoMapping and StrutsTypeConverterHolder's overrides plus the unknownMappings field (java:S6355). - Add the missing @deprecated Javadoc tag to the three StrutsTypeConverterHolder overrides, pointing at computeMappingIfAbsent as the replacement (java:S1123). - Remove the unused throws Exception from testGetConverterBuildsMappingExactlyOncePerClass (java:S1130). - Document why StubFileManager.setReloadingConfigs/monitorFile are intentionally empty no-ops (java:S1186). - Rename a local variable that shadowed the converter field in testConditionalReloadRebuildsEmptyMappingAndStoresItViaAddNoMapping (java:S1117). Co-Authored-By: Claude Opus 4.8 --- .../conversion/StrutsTypeConverterHolder.java | 20 +++++++++++++++---- .../conversion/TypeConverterHolder.java | 6 +++--- .../conversion/impl/XWorkConverterTest.java | 10 +++++++--- 3 files changed, 26 insertions(+), 10 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 95a8097831..ec8dddf3f6 100644 --- a/core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java +++ b/core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java @@ -84,7 +84,7 @@ public class StrutsTypeConverterHolder implements TypeConverterHolder { * for binary compatibility with subclasses compiled against earlier versions, and will be * removed in a future release. */ - @Deprecated + @Deprecated(since = "7.3.0", forRemoval = true) protected HashSet unknownMappings = new HashSet<>(); /** @@ -117,21 +117,33 @@ 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. + */ @Override - @Deprecated + @Deprecated(since = "7.3.0", forRemoval = true) public Map getMapping(Class clazz) { Map mapping = mappings.get(clazz); return mapping == NO_MAPPING ? null : mapping; } + /** + * @deprecated since 7.3.0, use {@link #computeMappingIfAbsent(Class, Function)} instead. + */ @Override - @Deprecated + @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. + */ @Override - @Deprecated + @Deprecated(since = "7.3.0", forRemoval = true) public boolean containsNoMapping(Class clazz) { return mappings.get(clazz) == NO_MAPPING; } 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 745595b992..15ccc2bdd9 100644 --- a/core/src/main/java/org/apache/struts2/conversion/TypeConverterHolder.java +++ b/core/src/main/java/org/apache/struts2/conversion/TypeConverterHolder.java @@ -63,7 +63,7 @@ public interface TypeConverterHolder { * @deprecated since 7.3.0, use {@link #computeMappingIfAbsent(Class, Function)} which resolves * and caches the mapping atomically instead of requiring a check-then-act at the call site. */ - @Deprecated + @Deprecated(since = "7.3.0", forRemoval = true) Map getMapping(Class clazz); /** @@ -74,7 +74,7 @@ public interface TypeConverterHolder { * @deprecated since 7.3.0, use {@link #computeMappingIfAbsent(Class, Function)} which stores * the built mapping itself. */ - @Deprecated + @Deprecated(since = "7.3.0", forRemoval = true) void addMapping(Class clazz, Map mapping); /** @@ -85,7 +85,7 @@ public interface TypeConverterHolder { * @deprecated since 7.3.0, use {@link #computeMappingIfAbsent(Class, Function)} which returns * an empty map for classes known to have no mapping. */ - @Deprecated + @Deprecated(since = "7.3.0", forRemoval = true) boolean containsNoMapping(Class clazz); /** 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 52d90df5fc..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 @@ -828,7 +828,7 @@ protected Map buildConverterMapping(Class clazz) throws Exceptio } } - public void testGetConverterBuildsMappingExactlyOncePerClass() throws Exception { + 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()); @@ -902,6 +902,8 @@ private static class StubFileManager implements FileManager { @Override public void setReloadingConfigs(boolean reloadingConfigs) { + // intentionally empty: this stub only controls fileNeedsReloading for the reload + // tests, and nothing in conditionalReload reads back the reloading-configs flag } @Override @@ -921,6 +923,8 @@ public InputStream loadFile(URL fileUrl) { @Override public void monitorFile(URL fileUrl) { + // intentionally empty: this stub only controls fileNeedsReloading for the reload + // tests, and nothing in conditionalReload depends on file monitoring being registered } @Override @@ -1008,9 +1012,9 @@ public void testConditionalReloadRebuildsEmptyMappingAndStoresItViaAddNoMapping( toggleConverter.setReloadingConfigs("true"); setFileManager(toggleConverter, new StubFileManager(true)); - Object converter = toggleConverter.getConverter(User.class, "someProperty"); + Object resolvedConverter = toggleConverter.getConverter(User.class, "someProperty"); - assertNull("the reload rebuilt an empty mapping, so no converter can be found", converter); + assertNull("the reload rebuilt an empty mapping, so no converter can be found", resolvedConverter); assertEquals("buildConverterMapping must run once for the initial cache miss (real mapping) " + "and once more for the forced reload (empty mapping)", 2, toggleConverter.calls.get()); From ebc0e8dff6649f4ae9e80bcc24d99bb3a22eaf84 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Tue, 21 Jul 2026 22:08:40 +0200 Subject: [PATCH 18/20] WW-5539 Suppress removal warnings for the deprecated holder primitives javac treats [removal] as a category separate from [deprecation], so marking the three primitives forRemoval left four warnings behind: the deliberate addMapping call in conditionalReload, and the three overrides that must exist for as long as the interface declares them. Co-Authored-By: Claude Opus 4.8 --- .../apache/struts2/conversion/StrutsTypeConverterHolder.java | 4 ++++ .../org/apache/struts2/conversion/impl/XWorkConverter.java | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) 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 ec8dddf3f6..4b3d5ca382 100644 --- a/core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java +++ b/core/src/main/java/org/apache/struts2/conversion/StrutsTypeConverterHolder.java @@ -123,6 +123,8 @@ public TypeConverter getDefaultMapping(String className) { * * @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) { @@ -133,6 +135,7 @@ public Map getMapping(Class clazz) { /** * @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) { @@ -142,6 +145,7 @@ public void addMapping(Class clazz, Map 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) { 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 3da17a7a4a..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 @@ -582,7 +582,7 @@ protected Map buildConverterMapping(Class clazz) throws Exceptio return mapping; } - @SuppressWarnings("deprecation") + @SuppressWarnings({"deprecation", "removal"}) private Map conditionalReload(Class clazz, Map oldValues) throws Exception { Map mapping = oldValues; From bd70047a17a444ab014b8de069a53ed2b0e86dad Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Wed, 22 Jul 2026 20:24:35 +0200 Subject: [PATCH 19/20] WW-5539 Address Copilot review comments Fixes getMapping's @return (it returns a Map, not a TypeConverter) and drops the "atomically" wording from its @deprecated tag, which no longer matches computeMappingIfAbsent's contract now that the builder may run more than once under concurrent first access. Syncs the design and plan docs with the shipped approach: the unknownMappings field is kept for binary compatibility rather than retyped, and the override uses get/build/putIfAbsent rather than computeIfAbsent. --- .../struts2/conversion/TypeConverterHolder.java | 6 +++--- ...2026-07-21-WW-5539-concurrency-performance.md | 16 +++++++++++----- ...-21-WW-5539-concurrency-performance-design.md | 12 +++++++----- 3 files changed, 21 insertions(+), 13 deletions(-) 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 15ccc2bdd9..8fc1c507ae 100644 --- a/core/src/main/java/org/apache/struts2/conversion/TypeConverterHolder.java +++ b/core/src/main/java/org/apache/struts2/conversion/TypeConverterHolder.java @@ -59,9 +59,9 @@ public interface TypeConverterHolder { * {@link #addMapping(Class, Map)}.

* * @param clazz class to convert to/from - * @return {@link TypeConverter} for given class - * @deprecated since 7.3.0, use {@link #computeMappingIfAbsent(Class, Function)} which resolves - * and caches the mapping atomically instead of requiring a check-then-act at the call site. + * @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); diff --git a/docs/superpowers/plans/2026-07-21-WW-5539-concurrency-performance.md b/docs/superpowers/plans/2026-07-21-WW-5539-concurrency-performance.md index 4858b474d2..bb4ef1dc68 100644 --- a/docs/superpowers/plans/2026-07-21-WW-5539-concurrency-performance.md +++ b/docs/superpowers/plans/2026-07-21-WW-5539-concurrency-performance.md @@ -34,7 +34,7 @@ This is the correctness fix. The holder is a container singleton whose plain `Ha **Interfaces:** - Consumes: nothing from earlier tasks. -- Produces: `StrutsTypeConverterHolder` with thread-safe internals and a `public StrutsTypeConverterHolder()` no-arg constructor (already implicit). Field `protected final Set unknownMappings`. Behaviour change: `addDefaultMapping(className, null)` is now ignored rather than storing a null value. +- Produces: `StrutsTypeConverterHolder` with thread-safe internals and a `public StrutsTypeConverterHolder()` no-arg constructor (already implicit). The original `protected HashSet unknownMappings` field is retained (deprecated, unused) for binary compatibility, with live storage in a new `private` concurrent set. Behaviour change: `addDefaultMapping(className, null)` is now ignored rather than storing a null value. - [ ] **Step 1: Write the failing concurrency test** @@ -534,13 +534,19 @@ Delete the `noMapping` field — the sentinel replaces it. Then: @Override public Map computeMappingIfAbsent(Class clazz, Function> builder) { - return mappings.computeIfAbsent(clazz, c -> { - Map built = builder.apply(c); - return (built == null || built.isEmpty()) ? NO_MAPPING : built; - }); + 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; } ``` +The builder deliberately runs **outside** `ConcurrentHashMap.computeIfAbsent`, via `get` → build → `putIfAbsent`. The builder instantiates arbitrary user-supplied `TypeConverter` classes (Spring-autowired under `SpringObjectFactory`), and running that under a CHM bin lock risks `IllegalStateException: Recursive update` or a self-deadlock if any of it re-enters conversion. The trade is that concurrent first access may build more than once; every caller still converges on the single stored mapping, and the build is idempotent. + `getMapping` and `containsNoMapping` translate the sentinel, so both keep their existing contracts — `getMapping` still returns `null` for an absent *or* negative-cached class. Add `@Deprecated` to the `getMapping`, `addMapping` and `containsNoMapping` overrides in this class so they do not emit "overrides deprecated method" noise. diff --git a/docs/superpowers/specs/2026-07-21-WW-5539-concurrency-performance-design.md b/docs/superpowers/specs/2026-07-21-WW-5539-concurrency-performance-design.md index 2482af6bba..57ab02604e 100644 --- a/docs/superpowers/specs/2026-07-21-WW-5539-concurrency-performance-design.md +++ b/docs/superpowers/specs/2026-07-21-WW-5539-concurrency-performance-design.md @@ -137,11 +137,13 @@ live external callers in `StrutsConversionPropertiesProcessor` and Removal of the deprecated methods is tracked separately by the project lead. -**The `protected HashSet unknownMappings` field** is retyped to `Set`, -made `final`, backed by `ConcurrentHashMap.newKeySet()`, and marked `@Deprecated`. -Retyping is a source-compatibility break for any subclass that assigns it or calls a -`HashSet`-specific method. The risk is judged negligible, but it is a break and is -recorded here deliberately rather than left implicit. +**The `protected HashSet unknownMappings` field** keeps its original +`protected HashSet` declaration, marked `@Deprecated(forRemoval = true)` and left +unused. Retyping it — the original plan — would have changed the field descriptor and thrown +`NoSuchFieldError` at runtime for any subclass compiled against an earlier version, a binary +break not permitted in a minor release. The live storage moves to a new `private` concurrent +set (`unknownMappingsInternal`, backed by `ConcurrentHashMap.newKeySet()`); the deprecated +field is retained solely for binary compatibility until its removal ticket lands. ### 2. `XWorkConverter` From 373d38dd76748b9b5721f6efedc2c429cb54ccaa Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 23 Jul 2026 09:19:44 +0200 Subject: [PATCH 20/20] WW-5539 docs: correct the classloader out-of-scope note The conversion caches are container-scoped singletons with no external references, so their Class keys do not independently pin the webapp classloader - that is governed by whatever retains the container (WW-5537). Reframed as optional defense-in-depth cache clearing, folded into WW-5537 Task 5b, rather than a standalone leak fix. --- ...-21-WW-5539-concurrency-performance-design.md | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/docs/superpowers/specs/2026-07-21-WW-5539-concurrency-performance-design.md b/docs/superpowers/specs/2026-07-21-WW-5539-concurrency-performance-design.md index 57ab02604e..dff7af895f 100644 --- a/docs/superpowers/specs/2026-07-21-WW-5539-concurrency-performance-design.md +++ b/docs/superpowers/specs/2026-07-21-WW-5539-concurrency-performance-design.md @@ -55,8 +55,8 @@ Two alternatives were considered and rejected: - **Striped per-key locking** (interning a lock object per class). Preserves exactly-once computation everywhere, but adds a lock-object cache that itself needs - eviction reasoning, still blocks readers behind writers, and duplicates the - classloader-pinning problem in a second map. `computeIfAbsent` provides the same + eviction reasoning, still blocks readers behind writers, and spreads the same + `Class`-keyed cache across a second map. `computeIfAbsent` provides the same guarantee where it matters, for free. - **Copy-on-write immutable snapshots.** Fastest possible reads, but `mappings` is keyed by every action class in the application, so each cold miss copies the whole map: @@ -307,10 +307,14 @@ the converter caches warm fast, and the validator lock is the coarser of the two ## Out of scope -1. **Classloader pinning.** `mappings` and `noMapping` hold strong `Class` references in a - container singleton, keeping webapp classloaders alive across hot redeploy. A separate - Jira issue: it is a lifetime/correctness issue rather than a locking one, and fixing it - changes cache semantics. +1. **Clearing the caches on cleanup (defense-in-depth).** `mappings` and the negative cache hold + strong references to application `Class` objects. This does **not** independently pin the webapp + classloader — the holder is a container-scoped singleton with no static, thread, or otherwise + external reference, so it is reachable only through the container and is collected together with it. + The classloader's survival is governed by whatever retains the *container*, which is out of scope + here and handled by the WW-5537 cleanup work. As a tidiness measure, clearing these caches during + `Dispatcher.cleanup()` (via `InternalDestroyable`) is folded into WW-5537 as Task 5b; it is not a + leak fix. 2. **The `FIXME lukaszlenart` in `TypeConverterHolder`** about merging `unknownMappings` into `noMapping` — a semantic consolidation, unrelated to locking. 3. **Removal of the deprecated methods** — tracked by the project lead for a later release.