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. -- 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]
