WW-5647 Use ConcurrentHashMap for XSLT template cache

[arunmanni-ai]

The XSLT template cache in XSLTResult uses a plain HashMap with an unsynchronized get() outside the synchronized block. This can produce stale reads or internal corruption under concurrent access.

This patch:

• Replaces HashMap with ConcurrentHashMap for thread-safe reads
• Adds double-check inside the synchronized block to avoid redundant template compilation
• Removes the static initializer in favour of inline initialization
• getTemplates() no longer writes to the shared cache when noCache=true, preventing a no-cache caller from polluting the cache for other callers.
• Added testConcurrentGetTemplatesCompilesOnce() and testNoCacheDoesNotPollutePersistentCache() to cover the double-check dedup behavior and the noCache isolation fix.

[lukaszlenart]

Code Review

Overview

Fixes a data race in XSLTResult.getTemplates(). The static templates cache was a plain HashMap read via an unsynchronized get() while writes happened inside a synchronized block. The PR swaps HashMap → ConcurrentHashMap, adds a double-checked read inside the synchronized block, and inlines the field initializer (dropping the static {} block).

The diagnosis is correct and the fix is sound. Concurrent read/write on a plain HashMap is a genuine bug (data corruption, and CPU-spinning infinite loops on older JDKs) — a legitimate correctness/hardening fix.

Correctness ✅

Traced the noCache interaction, the part most likely to break:

• noCache == false, concurrent miss: outer get() null → both threads enter synchronized sequentially → the double-check get() lets the second thread see the first thread's put() and skip recompilation. Exactly the redundant-compilation fix, and correct.
• noCache == true: the inner guard preserves noCache || (...), so it still recompiles every call. Behavior unchanged.
• Return value: templates is correctly reassigned on every path (cache hit, double-check hit, fresh compile), so the returned reference is always valid.

No regression in observable behavior; the only new effect is avoiding duplicate compilation on concurrent misses, which is the stated goal.

Style / Conventions ✅

• Inlining the initializer and removing the static {} block is a clean simplification.
• The re-check comment is helpful and matches surrounding style.

Suggestions / Minor Notes

• Pre-existing, not introduced here: with noCache == true the code still executes templatesCache.put(path, templates), polluting the shared static cache from a "no-cache" instance. Out of scope here, but worth a follow-up — arguably a noCache result should never populate the shared cache.
• synchronized (templatesCache) as a map lock is now redundant (the map is thread-safe), but it's still needed to serialize compilation and dedup work — keeping it is right. A computeIfAbsent alternative would hold a bin lock across the slow newTemplates() call, so the current approach is preferable. No change needed.
• Test coverage: XSLTResultTest.java exists but the PR adds no test. Concurrency is hard to test deterministically, but the double-check dedup behavior is testable — e.g. a subclass counting newTemplates invocations to assert a single path is compiled once. Optional but would lock in the intent.

Security

The underlying HashMap race could theoretically spin CPU under concurrent access, but this is defensive concurrency hardening on an internal template cache, not a classic Struts attack surface (OGNL/params/upload). Reasonable as a public PR.

Verdict

Approve. Small, correct, well-reasoned fix. The noCache-populates-cache observation and a dedup unit test are the only follow-ups worth considering, both optional.

🤖 Generated with Claude Code

[arunmanni-ai]

Hi @lukaszlenart , Addressed both optional follow-ups from your review:

1. noCache no longer pollutes the shared cache — getTemplates() now only calls templatesCache.put() when !noCache. A noCache=true caller still recompiles fresh every time (unchanged), it just no longer overwrites the cached entry other, cached callers rely on.

2. Dedup unit test added — testConcurrentGetTemplatesCompilesOnce() races two threads through a concurrent miss on the same path and asserts newTemplates() is invoked exactly once and both threads observe the same Templates instance.

Also added testNoCacheDoesNotPollutePersistentCache() to cover (1) directly: compiles once (cached), compiles again with noCache=true (different instance, proving it recompiles), then confirms a separate noCache=false caller still gets the original cached instance back.

Full suite: mvn -pl plugins/xslt -am test -Dtest=XSLTResultTest→ 17/17 passing (15 existing + 2 new).