mirror of
https://github.com/apache/struts.git
synced 2026-08-31 19:35:40 +00:00
Lukasz Lenart
8a5323fbdf
* WW-5539 docs: add concurrency performance enhancements design Design for removing coarse locks from XWorkConverter, DefaultActionValidatorManager and StrutsTypeConverterHolder in favour of concurrent collections. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5539 docs: add concurrency performance implementation plan Five tasks derived from the approved design: make StrutsTypeConverterHolder concurrent, add the computeMappingIfAbsent SPI method, remove the locks from XWorkConverter and DefaultActionValidatorManager, then benchmark and raise the PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5539 docs: make conditionalReload store its rebuilt mapping buildConverterMapping no longer stores its result, so reload mode would have rebuilt from disk on every request without ever caching. * 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. * WW-5539 Rename test to match what it actually covers The method exercised only the unknown-mapping cache, not noMapping. * WW-5539 Add TypeConverterHolder#computeMappingIfAbsent Adds an atomic build-once-and-cache operation so callers no longer need check-then-act around the class mapping cache, and deprecates the three primitives it subsumes: getMapping, addMapping and containsNoMapping. The method is a default method delegating to those primitives, so third-party TypeConverterHolder implementations keep working unchanged. * WW-5539 Deduplicate the no-mapping path in computeMappingIfAbsent ConcurrentHashMap.computeIfAbsent stores nothing when the mapping function returns null, so every concurrent caller re-ran the builder for a class with no conversion mapping - the common case for an ordinary action, and the exact thundering herd this method exists to prevent. Negative results now store a sentinel in the same map, so the builder runs once per class either way. getMapping and containsNoMapping translate the sentinel, preserving their existing contracts. * WW-5539 docs: sync plan with negative-cache sentinel fix * WW-5539 Pin down addNoMapping's override semantics Storing the no-mapping sentinel deliberately replaces any mapping cached for the class, matching the pre-7.3.0 effective behaviour where such a class was short-circuited before its cached mapping was ever read. putIfAbsent would instead serve a stale mapping after a failed build. Also asserts the sentinel translation in getMapping directly, and stops the interface javadoc promising a specific empty-map instance that implementations are not required to return. * WW-5539 Document that addNoMapping may replace a cached mapping The behaviour was documented only on the Struts implementation, but addNoMapping stays a non-deprecated SPI primitive that third parties both call and implement, so the contract belongs on the interface. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5539 Remove coarse locks from XWorkConverter getConverter() synchronized on the Class object being converted, which is a globally visible monitor any other library may contend on, and which serialised every conversion for a given action class including cache hits. It now delegates to TypeConverterHolder#computeMappingIfAbsent. registerConverter and registerConverterNotFound drop their synchronized modifier; they are single delegations to a concurrent map, and the lock never covered the readers in lookup() in any case. buildConverterMapping no longer stores its result - storage is owned by computeMappingIfAbsent. * WW-5539 Remove global lock from DefaultActionValidatorManager getValidators() was synchronized on the singleton manager, so every validated request in the application serialised on it - and the lock covered the per-request Validator construction loop, which operates on per-request objects and never needed mutual exclusion. Both caches become ConcurrentHashMap and cached config lists are wrapped unmodifiable, since several threads now iterate them concurrently. * WW-5539 Make the validator concurrency test race a cold cache The test computed its expected count with a getValidators call before starting the threads, which warmed the cache and left all 16 workers on the fast path - never exercising first-build contention, the race the test is named for. Also drops an unused import and awaits executor termination. * WW-5539 Address final review findings Restores the protected unknownMappings field verbatim 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 moves to a private concurrent set. Also stops conditionalReload running for negative-cached classes, which had been costing a failed classloader resource scan per property per request in devMode, and restores the unknown-mapping clearing that the null-converter guard was skipping. * WW-5539 Fix concurrency regressions from coarse-lock removal Four correctness fixes surfaced in PR review of the concurrent-collections refactor: - StrutsTypeConverterHolder.addDefaultMapping: restore put-before-remove ordering. The inverted order let a concurrent XWorkConverter.lookup observe (unknown=false, default=false), sending it into lookupSuper() and letting it overwrite the more specific converter being registered. - StrutsTypeConverterHolder.computeMappingIfAbsent: stop building inside a ConcurrentHashMap bin lock. The builder reaches ObjectFactory.buildConverter, which can autowire arbitrary user TypeConverters; running that under a CHM bin lock risked a recursive-update exception or self-deadlock. Callers now only get the guarantee that they converge on the same cached instance, not that the builder runs exactly once - documented on the interface and reflected in the concurrency tests. - DefaultValidatorFactory.validators: switch to ConcurrentHashMap now that DefaultActionValidatorManager.getValidators is no longer synchronized, so runtime registerValidator() calls no longer race unsynchronized reads of a plain HashMap. - XWorkConverter.conditionalReload: route empty devMode rebuilds through addNoMapping instead of addMapping, so an empty reload result is stored as the NO_MAPPING sentinel rather than a plain empty map that would silently disable further reloads for the class. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5539 Add tests closing coverage gaps from coarse-lock removal SonarCloud's quality gate failed at 59.8% coverage on new code (need >=80%). Adds tests for the specific lines JaCoCo identified as uncovered, without touching production code: - TypeConverterHolder.computeMappingIfAbsent's default method body (the SPI compatibility fallback for third-party holders that predate 7.3.0 and don't override it) - new TypeConverterHolderTest against a minimal non-overriding implementation. - StrutsTypeConverterHolder.getMapping/containsNoMapping's remaining non-sentinel branch. - XWorkConverter.conditionalReload's reloadingConfigs==true path (both the addMapping and addNoMapping outcomes), buildConverterMappingUnchecked's checked-to-IllegalStateException wrapping, and getConverter's catch(Throwable) negative-caching. - DefaultActionValidatorManager's else-if(reloadingConfigs) cache rebuild, loadFile's checkFile&&fileNeedsReloading re-parse, and buildValidatorConfigs' already-checked short-circuit. TypeConverterHolder.java and StrutsTypeConverterHolder.java are now at 0 missed lines/branches. XWorkConverter.java and DefaultActionValidatorManager.java have all requested target lines covered; remaining misses are pre-existing, unrelated gaps left alone per scope. Full core suite: 3026 tests (3015 + 11 new), 0 failures/errors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5539 Fix SonarCloud deprecation and test-hygiene issues - Add since/forRemoval attributes to the 7 @Deprecated elements on TypeConverterHolder.getMapping/addMapping/containsNoMapping and StrutsTypeConverterHolder's overrides plus the unknownMappings field (java:S6355). - Add the missing @deprecated Javadoc tag to the three StrutsTypeConverterHolder overrides, pointing at computeMappingIfAbsent as the replacement (java:S1123). - Remove the unused throws Exception from testGetConverterBuildsMappingExactlyOncePerClass (java:S1130). - Document why StubFileManager.setReloadingConfigs/monitorFile are intentionally empty no-ops (java:S1186). - Rename a local variable that shadowed the converter field in testConditionalReloadRebuildsEmptyMappingAndStoresItViaAddNoMapping (java:S1117). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5539 Suppress removal warnings for the deprecated holder primitives javac treats [removal] as a category separate from [deprecation], so marking the three primitives forRemoval left four warnings behind: the deliberate addMapping call in conditionalReload, and the three overrides that must exist for as long as the interface declares them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5539 Address Copilot review comments Fixes getMapping's @return (it returns a Map, not a TypeConverter) and drops the "atomically" wording from its @deprecated tag, which no longer matches computeMappingIfAbsent's contract now that the builder may run more than once under concurrent first access. Syncs the design and plan docs with the shipped approach: the unknownMappings field is kept for binary compatibility rather than retyped, and the override uses get/build/putIfAbsent rather than computeIfAbsent. * WW-5539 docs: correct the classloader out-of-scope note The conversion caches are container-scoped singletons with no external references, so their Class keys do not independently pin the webapp classloader - that is governed by whatever retains the container (WW-5537). Reframed as optional defense-in-depth cache clearing, folded into WW-5537 Task 5b, rather than a standalone leak fix. --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>