[
https://issues.apache.org/jira/browse/WW-5647?focusedWorklogId=1030899&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1030899
]
ASF GitHub Bot logged work on WW-5647:
--------------------------------------
Author: ASF GitHub Bot
Created on: 17/Jul/26 10:36
Start Date: 17/Jul/26 10:36
Worklog Time Spent: 10m
Work Description: lukaszlenart commented on PR #1781:
URL: https://github.com/apache/struts/pull/1781#issuecomment-5002231569
## 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](https://claude.com/claude-code)
Issue Time Tracking
-------------------
Worklog Id: (was: 1030899)
Remaining Estimate: 0h
Time Spent: 10m
> Use ConcurrentHashMap for XSLT template cache
> ---------------------------------------------
>
> Key: WW-5647
> URL: https://issues.apache.org/jira/browse/WW-5647
> Project: Struts 2
> Issue Type: Improvement
> Components: Plugin - XSLT
> Affects Versions: 7.2.1
> Reporter: Arun Manni
> Priority: Minor
> Fix For: 7.3.0
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> The XSLT template cache in XSLTResult uses a plain HashMap with an
> unsynchronized get() outside the synchronized block. Under concurrent
> access this can produce stale reads or exceptions from internal
> HashMap corruption.
> This patch replaces HashMap with ConcurrentHashMap for thread-safe
> reads, adds a double-check inside the synchronized block to avoid
> redundant template compilation, and removes the static initializer
> in favour of inline initialization.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
