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