[
https://issues.apache.org/jira/browse/WW-5539?focusedWorklogId=1031730&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1031730
]
ASF GitHub Bot logged work on WW-5539:
--------------------------------------
Author: ASF GitHub Bot
Created on: 22/Jul/26 18:32
Start Date: 22/Jul/26 18:32
Worklog Time Spent: 10m
Work Description: 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.
Issue Time Tracking
-------------------
Worklog Id: (was: 1031730)
Time Spent: 1h 10m (was: 1h)
> Concurrency performance enhancements
> ------------------------------------
>
> Key: WW-5539
> URL: https://issues.apache.org/jira/browse/WW-5539
> Project: Struts 2
> Issue Type: Improvement
> Components: Core
> Affects Versions: 6.7.4, 7.0.3
> Reporter: Kusal Kithul-Godage
> Assignee: Lukasz Lenart
> Priority: Minor
> Fix For: 7.3.0
>
> Time Spent: 1h 10m
> Remaining Estimate: 0h
>
> We've identified some code which can be made more performant by improving the
> locking mechanism in the following classes:
> * XWorkConverter
> * DefaultActionValidatorManager
> * StrutsTypeConverterHolder
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
