lukaszlenart commented on code in PR #1799:
URL: https://github.com/apache/struts/pull/1799#discussion_r3632811731
##########
core/src/main/java/org/apache/struts2/conversion/TypeConverterHolder.java:
##########
@@ -52,29 +54,44 @@ public interface TypeConverterHolder {
/**
* Target class conversion Mappings.
*
+ * <p>Returns {@code null} if the class has been flagged as having no
mapping via
+ * {@link #addNoMapping(Class)}, even if a real mapping was previously
stored for it with
+ * {@link #addMapping(Class, Map)}.</p>
+ *
* @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.
Review Comment:
Fixed in bd70047a1 — `@return` now reads "the property-converter mapping for
the given class, or `null` if none" (it was incorrectly copied from a
`TypeConverter`-returning method), and the "atomically" wording is dropped from
the `@deprecated` tag. Good catch that it contradicted the reworded
`computeMappingIfAbsent` contract, where the builder may run more than once
under concurrent first access.
##########
core/src/main/java/org/apache/struts2/conversion/TypeConverterHolder.java:
##########
@@ -97,4 +114,42 @@ public interface TypeConverterHolder {
*/
void addUnknownMapping(String className);
+ /**
+ * Returns the property-converter mapping for the given class, building
and caching it on first
+ * use. Never returns {@code null}: a class known to have no mapping
yields an empty map.
+ *
+ * <p>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.</p>
+ *
+ * <p>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.</p>
+ *
+ * @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")
Review Comment:
Verified this one and it does not reproduce — leaving as-is. The `default`
body calls `getMapping`/`addMapping`/`containsNoMapping`, which are declared in
this same interface (`TypeConverterHolder`). Per JLS 9.6.4.6, a use within the
same outermost class as the deprecated element produces no warning at all —
neither `deprecation` nor `removal` — so there is nothing for `removal` to
suppress here (the existing `deprecation` suppression is in fact redundant on
this method). Confirmed by compiling `core` with lint active: 44 `[removal]`
warnings across the module, none in `TypeConverterHolder.java`. The genuine
cross-class site, `XWorkConverter.conditionalReload`, was widened to
`@SuppressWarnings({"deprecation","removal"})` in ebc0e8dff.
##########
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<String, Object> computeMappingIfAbsent(Class clazz,
+ Function<Class, Map<String,
Object>> 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<String, Object> mapping = mappings.computeIfAbsent(clazz, c -> {
+ Map<String, Object> 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<String> unknownMappings` field** is retyped to
`Set<String>`,
+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.
Review Comment:
Fixed in bd70047a1 — the design doc now describes the shipped approach:
`unknownMappings` keeps its original `protected HashSet<String>` declaration
(deprecated, unused) for binary compatibility, with live storage in a private
`ConcurrentHashMap.newKeySet()`. Retyping it, as the doc originally proposed,
would have changed the field descriptor and thrown `NoSuchFieldError` for
subclasses compiled against an earlier version.
##########
docs/superpowers/plans/2026-07-21-WW-5539-concurrency-performance.md:
##########
@@ -0,0 +1,1209 @@
+# 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<String> unknownMappings`. Behaviour change:
`addDefaultMapping(className, null)` is now ignored rather than storing a null
value.
Review Comment:
Fixed in bd70047a1 — the plan's "Produces" line now reflects that the
original `protected HashSet<String> unknownMappings` field is retained
(deprecated, unused) for binary compatibility, with live storage in a new
private concurrent set.
##########
docs/superpowers/plans/2026-07-21-WW-5539-concurrency-performance.md:
##########
@@ -0,0 +1,1209 @@
+# 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<String> 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<Future<?>> futures = new ArrayList<>();
+
+ for (int t = 0; t < THREADS; t++) {
+ final int threadId = t;
+ futures.add(pool.submit(() -> {
+ start.await();
+ for (int i = 0; i < PER_THREAD; i++) {
+ String className = "stub.Class" + threadId + "_" + i;
+ holder.addDefaultMapping(className, stubConverter());
+ // interleave reads with writes to provoke the race
+ holder.containsDefaultMapping("stub.Class0_0");
+ holder.getDefaultMapping(className);
+ }
+ return null;
+ }));
+ }
+
+ start.countDown();
+ for (Future<?> future : futures) {
+ future.get(60, TimeUnit.SECONDS);
+ }
+ pool.shutdown();
+
+ for (int t = 0; t < THREADS; t++) {
+ for (int i = 0; i < PER_THREAD; i++) {
+ String className = "stub.Class" + t + "_" + i;
+ assertThat(holder.getDefaultMapping(className))
+ .as("lost registration for %s", className)
+ .isNotNull();
+ }
+ }
+ }
+
+ public void
testConcurrentNoMappingAndUnknownMappingRegistrationLosesNothing() throws
Exception {
+ StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder();
+ ExecutorService pool = Executors.newFixedThreadPool(THREADS);
+ CountDownLatch start = new CountDownLatch(1);
+ List<Future<?>> futures = new ArrayList<>();
+
+ for (int t = 0; t < THREADS; t++) {
+ final int threadId = t;
+ futures.add(pool.submit(() -> {
+ start.await();
+ for (int i = 0; i < PER_THREAD; i++) {
+ holder.addUnknownMapping("stub.Unknown" + threadId + "_" +
i);
+ holder.containsUnknownMapping("stub.Unknown0_0");
+ }
+ return null;
+ }));
+ }
+
+ start.countDown();
+ for (Future<?> future : futures) {
+ future.get(60, TimeUnit.SECONDS);
+ }
+ pool.shutdown();
+
+ for (int t = 0; t < THREADS; t++) {
+ for (int i = 0; i < PER_THREAD; i++) {
+ String className = "stub.Unknown" + t + "_" + i;
+ assertThat(holder.containsUnknownMapping(className))
+ .as("lost unknown mapping for %s", className)
+ .isTrue();
+ }
+ }
+ }
+
+ public void testAddDefaultMappingIgnoresNullConverter() {
+ StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder();
+
+ holder.addDefaultMapping("stub.NullConverter", null);
+
+
assertThat(holder.containsDefaultMapping("stub.NullConverter")).isFalse();
+ assertThat(holder.getDefaultMapping("stub.NullConverter")).isNull();
+ }
+
+ public void testAddDefaultMappingClearsUnknownMapping() {
+ StrutsTypeConverterHolder holder = new StrutsTypeConverterHolder();
+
+ holder.addUnknownMapping("stub.Later");
+ assertThat(holder.containsUnknownMapping("stub.Later")).isTrue();
+
+ holder.addDefaultMapping("stub.Later", stubConverter());
+
+ assertThat(holder.containsUnknownMapping("stub.Later")).isFalse();
+ assertThat(holder.getDefaultMapping("stub.Later")).isNotNull();
+ }
+}
+```
+
+- [ ] **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<String, TypeConverter> defaultMappings = new
ConcurrentHashMap<>(); // non-action (eg. returned value)
+```
+
+```java
+ private final Map<Class, Map<String, Object>> mappings = new
ConcurrentHashMap<>(); // action
+```
+
+```java
+ private final Set<Class> noMapping = ConcurrentHashMap.newKeySet(); //
action
+```
+
+```java
+ /**
+ * Record classes that doesn't have conversion mapping defined.
+ * <pre>
+ * - String -> classname as String
+ * </pre>
+ *
+ * @deprecated since 7.3.0, this field is an implementation detail and
will be made private.
+ */
+ @Deprecated
+ protected final Set<String> 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<String, Object>
TypeConverterHolder.computeMappingIfAbsent(Class clazz, Function<Class,
Map<String, Object>> 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<String, Object> first =
holder.computeMappingIfAbsent(String.class, clazz -> {
+ builds.incrementAndGet();
+ Map<String, Object> built = new HashMap<>();
+ built.put("someProperty", "someConverter");
+ return built;
+ });
+ Map<String, Object> 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<String, Object> first =
holder.computeMappingIfAbsent(String.class, clazz -> {
+ builds.incrementAndGet();
+ return Collections.emptyMap();
+ });
+ Map<String, Object> 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<String, Object> 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<String, Object> 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<Future<Map<String, Object>>> 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<String, Object> built = new HashMap<>();
+ built.put("someProperty", "someConverter");
+ return built;
+ });
+ }));
+ }
+
+ start.countDown();
+ Map<String, Object> expected = futures.get(0).get(60,
TimeUnit.SECONDS);
+ for (Future<Map<String, Object>> 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<String, Object> 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<String, Object> 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()}.
+ *
+ * <p>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.</p>
+ *
+ * <p>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.</p>
+ *
+ * @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<String, Object> computeMappingIfAbsent(Class clazz,
Function<Class, Map<String, Object>> builder) {
+ if (containsNoMapping(clazz)) {
+ return Collections.emptyMap();
+ }
+ Map<String, Object> 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:
+
+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<String, Object> NO_MAPPING =
Collections.unmodifiableMap(new HashMap<>());
+```
+
+Delete the `noMapping` field — the sentinel replaces it. Then:
+
+```java
+ @Override
+ @Deprecated
+ public Map<String, Object> getMapping(Class clazz) {
+ Map<String, Object> 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<String, Object> computeMappingIfAbsent(Class clazz,
Function<Class, Map<String, Object>> builder) {
+ return mappings.computeIfAbsent(clazz, c -> {
+ Map<String, Object> built = builder.apply(c);
+ return (built == null || built.isEmpty()) ? NO_MAPPING : built;
+ });
+ }
Review Comment:
Fixed in bd70047a1 — the plan snippet now shows the shipped `get` → build →
`putIfAbsent` form, with a note explaining why the builder deliberately runs
outside `computeIfAbsent` (it instantiates arbitrary, Spring-autowired
`TypeConverter` classes, which under a CHM bin lock risks
`IllegalStateException: Recursive update` or self-deadlock) and that the
trade-off is a possible duplicate build under concurrent first access.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
