WW-5539 Concurrency performance enhancements

[lukaszlenart]

Fixes 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 get → build → putIfAbsent, storing a sentinel for negative results so a class with no mapping is cached as decisively as one with 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.

The builder is deliberately not run inside ConcurrentHashMap.computeIfAbsent. It reaches ObjectFactory.buildConverter, which instantiates and (under SpringObjectFactory) autowires arbitrary user-supplied TypeConverter classes — constructors, @PostConstruct, afterPropertiesSet. Running that under a CHM bin lock risks IllegalStateException: Recursive update or a self-deadlock if any of it re-enters conversion, and DefaultConversionAnnotationProcessor already demonstrates that the builder calls back into the holder. The trade is that concurrent first access may build more than once; every caller still converges on a single shared cached map, and the build is idempotent.

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.

Also fixed here

DefaultValidatorFactory.validators is a plain HashMap read via lookupRegisteredValidatorType on every validator construction. That read used to be serialised by DefaultActionValidatorManager's lock; removing the lock exposed it, and ValidatorFactory.registerValidator is public API that writes to it. Changed to ConcurrentHashMap — otherwise this PR would have pushed the very hazard it removes down one layer.

Testing

Full core module: 3026 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.

[sonarqubecloud]

Quality Gate passed

Issues
19 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud