fix: synchronize ReflectionValueExtractor class-introspection cache for thread safety

[waterWang]

Problem

ReflectionValueExtractor keeps a static WeakHashMap class-introspection cache that is read and written concurrently from multiple builder threads under -T (parallel builds). WeakHashMap is not thread-safe — concurrent resize from multiple threads can create circular bucket chains, causing the build to hang indefinitely at 100% CPU.

Observed symptom: mvn -T1C hangs with one core pinned at 100%, thread dump shows WeakHashMap.transfer in a RUNNABLE state inside getClassMap().

Fix

Wrap the WeakHashMap with Collections.synchronizedMap() to serialize all reads and writes. This preserves the weak-reference semantics (classloader GC is unaffected) while providing thread safety.

Changes

• ReflectionValueExtractor.java: wrap CLASS_MAPS with Collections.synchronizedMap(), add import java.util.Collections

Fixes #12731

[gnodet]

Correct minimal fix for a real production hang. Wrapping the unsynchronized WeakHashMap with Collections.synchronizedMap() prevents concurrent resize()/transfer() corruption under -T parallel builds.

The remaining TOCTOU race in getClassMap (non-atomic get-null-check-put) is benign since ClassMap construction is idempotent — a duplicate creation is wasted work but not a correctness issue.

Non-blocking observation:

The deprecated compat copy at compat/maven-model-builder/src/main/java/org/apache/maven/model/interpolation/reflection/ReflectionValueExtractor.java (line 54) has the identical unsynchronized WeakHashMap pattern. While the main code path goes through the impl version (which this PR fixes), the compat copy is still reachable via ObjectBasedValueSource and could theoretically trigger the same hang under -T. Consider applying the same fix there for completeness.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[knowledge-graphlet]

Thank you :-)

[kec]

Reporter of #12731 here. Two things that might help this land in a release.

Milestone. This PR is currently unmilestoned — would 4.0.0-rc-7 (milestone 131) be reasonable? The failure mode is an indefinite hang with no output and no exception under -T, so in unattended CI an affected build holds an agent until something external kills it; there is nothing to react to. We hit it twice on rc-5 — once as this hang, once weeks earlier as a ConcurrentModificationException from the same call path — and rc-6 carries the same code.

Completing the fix. @gnodet's review flagged that the deprecated compat copy at compat/maven-model-builder/src/main/java/org/apache/maven/model/interpolation/reflection/ReflectionValueExtractor.java:54 has the identical unsynchronized WeakHashMap and the same non-atomic getClassMap, still reachable through ObjectBasedValueSource. I have that change ready in exactly the form used here. Happy to send it either way:

• as a PR against waterWang:fix/reflection-weak-hashmap-concurrency, keeping this a single commit, or
• as a companion PR once this one merges.

@waterWang — your call; say which you'd prefer and I'll open it.

I can also attach the full thread dump from the original incident to #12731 if that is useful for the record.

[gnodet]

I'd rather have a single PR

[kec]

@gnodet understood — single PR it is.

I've sent the compat copy to @waterWang's branch as waterWang#1. Merging it there folds both files into this PR, so apache/maven still sees one PR rather than a follow-up.

@waterWang — it's the same two-line change as yours, applied to compat/maven-model-builder/.../interpolation/reflection/ReflectionValueExtractor.java. Merge at your convenience and this PR picks it up automatically; no action needed from me after that.

[gnodet]

COMMENT — The synchronization fix for the impl copy is correct and minimal. Wrapping the unsynchronized WeakHashMap with Collections.synchronizedMap() prevents the infinite-loop hang caused by concurrent resize/transfer corruption under -T parallel builds.

However, as @gnodet noted, the PR remains incomplete:

Missing compat copy fix (high): The identical unsynchronized WeakHashMap exists in compat/maven-model-builder/.../ReflectionValueExtractor.java at line 54, with the same non-thread-safe getClassMap() pattern at lines 292-303. This copy is still reachable via ObjectBasedValueSource and can trigger the same infinite-loop hang under -T. @gnodet requested both fixes in a single PR ("I'd rather have a single PR"). @kec sent the fix to waterWang#1, but that PR is still open and has not been merged into this PR's branch.

Benign TOCTOU race (low, no action needed): The getClassMap() method does CLASS_MAPS.get() → null check → CLASS_MAPS.put(). These are individually atomic under synchronizedMap, but the compound operation is not. This is benign because ClassMap construction is deterministic and idempotent — the worst case is duplicate construction (wasted CPU, not a correctness issue).

This review was generated by an AI agent (Claude Code) and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of Guillaume Nodet