[
https://issues.apache.org/jira/browse/WW-5539?focusedWorklogId=1031535&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1031535
]
ASF GitHub Bot logged work on WW-5539:
--------------------------------------
Author: ASF GitHub Bot
Created on: 21/Jul/26 18:56
Start Date: 21/Jul/26 18:56
Worklog Time Spent: 10m
Work Description: lukaszlenart opened a new pull request, #1799:
URL: https://github.com/apache/struts/pull/1799
Fixes [WW-5539](https://issues.apache.org/jira/browse/WW-5539)
Three core classes had coarse locks compensating for caches that were not
thread-safe. This makes the caches genuinely concurrent and deletes the locks.
## What was wrong
**`StrutsTypeConverterHolder`** — a container singleton whose four
`HashMap`/`HashSet` caches were read with no lock at all by
`XWorkConverter.lookup()` while being written elsewhere. This is a real data
race, not just contention. The new test reproduces it against the unfixed code:
a lost registration, and a `HashMap$Node`→`HashMap$TreeNode`
`ClassCastException` thrown from concurrent structural modification during
resize.
**`XWorkConverter`** — `getConverter()` held `synchronized (clazz)`. Locking
on a `Class` object is a globally visible monitor that any unrelated library
can also lock, and it serialised every conversion for a given action class,
including pure cache hits. `registerConverter`/`registerConverterNotFound` were
`synchronized` methods on the singleton, though they only ever excluded each
other — the readers in `lookup()` were never covered.
**`DefaultActionValidatorManager`** — `getValidators()` was `synchronized`
on the singleton, so every validated request in the application serialised on
it. The lock also covered the per-request `Validator` construction loop, which
operates on per-request objects and never needed mutual exclusion.
## Approach
Concurrent collections, then the locks come out. Where a computation is
expensive and provably safe to guard, `computeIfAbsent`; where it is not,
check-then-act with cheap idempotent duplicate work.
`TypeConverterHolder` gains one `default` method,
`computeMappingIfAbsent(Class, Function)`, so third-party implementations of
this SPI keep working unchanged. `StrutsTypeConverterHolder` overrides it with
a real `computeIfAbsent`, storing a sentinel for negative results so the
builder runs exactly once per class whether or not the class has a mapping —
the no-mapping case is the common one for ordinary actions, and the builder
walks the whole class hierarchy doing classpath lookups and reflection.
## Measurements
Throughput, 3s measured after 20k warm-up iterations, on the same machine,
before vs after. Second of two runs each; both runs agreed closely.
`DefaultActionValidatorManager.getValidators`:
| threads | before (ops/s) | after (ops/s) | ratio |
|--------:|---------------:|--------------:|------:|
| 1 | 11,100 | 11,300 | 1.0x |
| 4 | 10,400 | 41,866 | 4.0x |
| 16 | 10,700 | 77,366 | 7.2x |
| 64 | 12,100 | 77,700 | 6.4x |
The before column is the signature of a global lock: flat at ~10k ops/s no
matter how many threads you add. This is the headline change.
`XWorkConverter.getConverter`:
| threads | before (ops/s) | after (ops/s) | ratio |
|--------:|---------------:|--------------:|------:|
| 1 | 26,342,966 | 94,234,166 | 3.6x |
| 4 | 13,362,366 | 341,507,566 | 25.6x |
| 16 | 12,643,300 | 850,345,433 | 67x |
| 64 | 12,257,333 | 847,833,400 | 69x |
Treat the converter *magnitudes* with suspicion — a warm cache hit is a
single map lookup whose result the harness discards, so the JIT can optimise
aggressively and the absolute figures are inflated. The trustworthy signal is
the **shape**: before, throughput *falls* from 26M to 13M as soon as a second
thread appears (negative scaling under `synchronized (clazz)`); after, it
scales with thread count. The single-threaded 3.6x is the uncontended-lock
cost, which is real now that biased locking is gone from modern JVMs.
The benchmark harness was throwaway and is not included in this PR.
## Behaviour changes
- `XWorkConverter.buildConverterMapping` (`protected`) no longer stores its
result; storage is owned by `computeMappingIfAbsent`. `conditionalReload`
writes back its own rebuild.
- `StrutsTypeConverterHolder.addDefaultMapping` now ignores a `null`
converter with a warning instead of storing it. `ConcurrentHashMap` forbids
null values, and storing one previously left the holder inconsistent —
`containsDefaultMapping` true while `getDefaultMapping` returned null. No
in-tree caller can pass null today.
- `addNoMapping` storing the sentinel replaces any mapping previously cached
for that class. This matches the pre-7.3.0 *observable* behaviour, where such a
class was short-circuited before its cached mapping was ever read.
`putIfAbsent` was considered and rejected: it would leave a stale mapping being
served after a failed build.
- `XWorkConverter.registerConverter`/`registerConverterNotFound` are no
longer `synchronized`.
- `TypeConverterHolder.getMapping`, `addMapping` and `containsNoMapping` are
deprecated, superseded by `computeMappingIfAbsent`. `addNoMapping` stays — it
is still the right primitive for recording a build failure, and folding it in
would mean swallowing `Throwable` inside the SPI.
Binary compatibility was deliberately preserved.
`StrutsTypeConverterHolder.unknownMappings` keeps its original `protected
HashSet<String>` declaration 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 moved to a
private concurrent set.
## Known benign race, deliberately not closed
`lookup()` reads `containsUnknownMapping` then `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. Adding a lock would reintroduce the contention this PR removes, on
the hottest read path, to close a window that resolves itself. Note that
against the old unsynchronised `HashMap` this same interleaving was outright
unsafe.
Relatedly, `lookup()` deliberately keeps check-then-act rather than using
`computeIfAbsent`: 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`.
## Testing
Full core module: 3014 tests, 0 failures, 0 errors. Full multi-module
reactor: BUILD SUCCESS. No pre-existing test needed modification.
New concurrency tests use 16 threads released simultaneously via
`CountDownLatch`, asserting on collected results rather than on timing. They
can demonstrate a race but never prove its absence — a green run on a
strongly-ordered machine is weak evidence, and the primary correctness argument
is the reasoning about the data structures: reads on concurrent collections,
both `computeIfAbsent` call sites verified not to re-enter their own maps, and
the remaining races benign and self-correcting.
## Follow-ups (not in this PR)
- **Classloader pinning.** `mappings` holds strong `Class` references in a
container singleton, keeping webapp classloaders alive across hot redeploy. A
lifetime/correctness issue rather than a locking one, and fixing it changes
cache semantics — deserves its own ticket.
- **Removing the deprecated methods** in a later release.
- The `FIXME lukaszlenart` in `TypeConverterHolder` about merging
`unknownMappings` into `noMapping` is a semantic consolidation, untouched here.
Issue Time Tracking
-------------------
Worklog Id: (was: 1031535)
Remaining Estimate: 0h
Time Spent: 10m
> 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: 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)
