[jira] [Commented] (GROOVY-11966) AnnotationNode.isTargetAllowed introduces concurrent write to shared ListHashMap

["Paul King (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/GROOVY-11966?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18076316#comment-18076316 ] 

Paul King commented on GROOVY-11966:
------------------------------------

The proposed PR has a test under the "stress" subproject. Run with:

{noformat}
./gradlew -PstressTests :stress:stressTest --tests org.codehaus.groovy.ast.NodeMetaDataHandlerStressTest                                                                         {noformat}

Verified bidirectional:
* Without the fix: test fails fast with 16 errors — NullPointerException: ... "this.innerMap" is null from ListHashMap.remove, exactly the resize race.
* With the fix: passes in ~75ms.

> AnnotationNode.isTargetAllowed introduces concurrent write to shared ListHashMap
> --------------------------------------------------------------------------------
>
>                 Key: GROOVY-11966
>                 URL: https://issues.apache.org/jira/browse/GROOVY-11966
>             Project: Groovy
>          Issue Type: Bug
>            Reporter: Paul King
>            Assignee: Paul King
>            Priority: Major
>
> h3. Summary
> {{AnnotationNode.isTargetAllowed}} (and {{getRetentionPolicy}}) lazily caches its result via {{classNode.redirect().getNodeMetaData(Target.class, factory)}}, which delegates to {{NodeMetaDataHandler.getNodeMetaData(Object, Function)}} and ultimately calls {{computeIfAbsent}} on a {{ListHashMap}}. {{ListHashMap}} is documented as not thread-safe.
> The {{ClassNode}} for built-in annotations like {{@CompileStatic}}, {{@Inject}}, {{@Target}}, etc. is shared process-wide via {{ClassHelper.makeCached}} (SoftReference cache keyed by {{Class}}). Parallel compiles in the same JVM (Gradle {{--parallel}}, multi-source-set builds, Grails worker pools) therefore race on the same metadata map.
> h3. Symptom
> {{ArrayIndexOutOfBoundsException}} from {{ListHashMap.toMap}} / {{ListHashMap.put}} during the array-to-{{HashMap}} resize. Concretely, one thread can finish the {{evolve}} branch and bump {{size}} past {{keys.length}} while another thread is mid-{{toMap}} reading {{keys[i]}}.
> h3. Affects
> Master only. Path was introduced by GROOVY-6526 (commit {{e16c9fb618}}) and is exercised on every annotation by GROOVY-11838's default-target enforcement. Groovy 5 returned a precomputed {{int}} field with no shared-state access.
> h3. Fix options
> * Add dedicated {{volatile int allowedTargets}} and {{volatile RetentionPolicy retentionPolicy}} fields to {{ClassNode}}; lazy-init from {{AnnotationNode}}, bypassing {{NodeMetaDataHandler}} for these two lookups. Idempotent computation, safe under races, no lock.
> * Switch {{NodeMetaDataHandler.newMetaDataMap()}} default to {{ConcurrentHashMap}} (atomic {{computeIfAbsent}}). Wider scope, fixes any similar latent races in other metadata callers.
> * Synchronise the read-modify-write in {{NodeMetaDataHandler.getNodeMetaData(Object, Function)}}.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)