GROOVY-11966: AnnotationNode.isTargetAllowed introduces concurrent wr…

[paulk-asert]

…ite to shared ListHashMap

https://issues.apache.org/jira/browse/GROOVY-11966

Background

NodeMetaDataHandler stores per-ASTNode metadata in a ListHashMap — a small-N optimised map that promotes its internal storage from arrays to a HashMap once it grows past three entries. This map is not thread-safe, and the array→HashMap promotion inside ListHashMap.put is the specific operation that races under concurrent access.

Built-in annotation ClassNodes such as @CompileStatic, @Inject, and @Target are cached process-wide via ClassHelper.makeCached. AnnotationNode.isTargetAllowed then memoises an allowed-targets bitmask on each cached node via classNode.redirect().getNodeMetaData(Target.class, factory). That call looks like a read but is actually a write into the shared ListHashMap.

Any toolchain that compiles Groovy sources in parallel within a single JVM hits this:

• Grails GSP compilation — see grails-core#15558. The GSP compiler used Executors.newFixedThreadPool(availableProcessors() * 2) and parallel compilation of pages annotated with @CompileStatic, @Inject, etc. reliably produced ArrayIndexOutOfBoundsException: Index 3 out of bounds for length 3 ... at org.codehaus.groovy.util.ListHashMap.toMap. Grails worked around it by defaulting GSP compiler parallelism to 1 on Groovy 6 (configurable via -Dgrails.gsp.compiler.parallelism=N) pending an upstream fix.
• Gradle parallel compilation, Maven multi-module builds, and any other multi-threaded consumer of groovy-ast are affected similarly.

The new reproducer under subprojects/stress exercises this directly on a shared cached ClassNode from 16 threads.

Fix

Switch the default newMetaDataMap() to return Collections.synchronizedMap(new ListHashMap()). Per-entry operations on the wrapper (get, put, remove, computeIfAbsent) become atomic on a private mutex inside the wrapper, eliminating the race on the array→HashMap promotion. Lazy first-creation of the map field uses a double-checked-locking idiom over a volatile metaDataMap field on ASTNode and CompileUnit. Iteration over the result of getMetaDataMap() is documented in Javadoc as a caller responsibility — Collections#synchronizedMap requires synchronized (map) { … } around iteration.

ConcurrentHashMap was the main alternative considered.

Trade-off comparison

	Groovy prior to any fix	Collections.synchronizedMap(new ListHashMap()) (this PR)	ConcurrentHashMap
Per-node memory overhead (when metadata is present)	0 (raw ListHashMap)	+24–32 bytes wrapper	much heavier than the synchronized wrapper; most metadata maps hold 0–3 entries where CHM is the wrong shape
Hot-path read cost	unsynchronised (unsafe)	volatile field read + monitor enter on wrapper's private mutex	volatile / CAS, no monitor
computeIfAbsent atomicity	not atomic	atomic (Collections#synchronizedMap overrides since JDK 8u)	atomic
getNodeMetaData() no-arg result	live unmodifiableMap over an unprotected ListHashMap (unsafe under contention)	Map.copyOf snapshot taken under the wrapper's mutex	Map.copyOf snapshot (lock-free traversal)
Iteration semantics on result of getMetaDataMap()	undefined under contention	caller must wrap iteration in synchronized (map) { … } per the Collections#synchronizedMap contract	weakly consistent — iterators reflect state at some point at or after creation, no ConcurrentModificationException
ArrayIndexOutOfBoundsException / NullPointerException from parallel access to ListHashMap (GROOVY-11966)	yes	eliminated	eliminated (different storage)
API caller multi-threading contract	implicit single-threaded; undocumented; widely violated by parallel-compile tooling	per-entry mutators are thread-safe; iteration of getMetaDataMap() requires caller-side synchronisation	per-entry mutators are thread-safe; iteration is weakly consistent

Extensibility hook

NodeMetaDataHandler.newMetaDataMap() (added in 5.0.0) remains the extension point for downstream consumers. AST-aware IDE/editor tooling that reuses AST nodes across long-lived sessions has historically overridden it to inject a thread-safe map. With this change, the default is already thread-safe so such overrides are no longer required for correctness; the hook now serves as a performance / customisation knob — for example, downstream consumers may still choose to override with ConcurrentHashMap for workloads that favour lock-free iteration, or with a differently-tuned ListHashMap for memory-constrained scenarios.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 67.1372%. Comparing base (7c36803) to head (7a7a5b7).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...a/org/codehaus/groovy/ast/NodeMetaDataHandler.java	95.0000%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##               master      #2492        +/-   ##
==================================================
- Coverage     67.1394%   67.1372%   -0.0022%     
+ Complexity      31627      31625         -2     
==================================================
  Files            1451       1451                
  Lines          122560     122561         +1     
  Branches        22007      22007                
==================================================
- Hits            82286      82284         -2     
- Misses          33194      33195         +1     
- Partials         7080       7082         +2     

Files with missing lines	Coverage Δ
src/main/java/org/codehaus/groovy/ast/ASTNode.java	93.5484% <ø> (ø)
...main/java/org/codehaus/groovy/ast/CompileUnit.java	74.2424% <ø> (ø)
...a/org/codehaus/groovy/ast/NodeMetaDataHandler.java	88.3721% <95.0000%> (-2.1041%)	⬇️

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.