[
https://issues.apache.org/jira/browse/GROOVY-11966?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18076426#comment-18076426
]
ASF GitHub Bot commented on GROOVY-11966:
-----------------------------------------
Copilot commented on code in PR #2492:
URL: https://github.com/apache/groovy/pull/2492#discussion_r3144678223
##########
subprojects/stress/src/stressTest/java/org/codehaus/groovy/ast/NodeMetaDataHandlerStressTest.java:
##########
@@ -0,0 +1,100 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.codehaus.groovy.ast;
+
+import org.apache.groovy.stress.util.ThreadUtils;
+import org.junit.Test;
+
+import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+
+import static org.junit.Assert.assertEquals;
+
+/**
+ * Exercises concurrent access to {@link NodeMetaDataHandler}'s default
methods on a
+ * shared AST node. The default {@code newMetaDataMap()} returns a {@code
ListHashMap},
+ * which is not thread-safe; without internal synchronization, concurrent
compiles
+ * sharing a {@link ClassNode} (e.g. built-in annotation nodes cached by
+ * {@link ClassHelper}) trip {@code ArrayIndexOutOfBoundsException} during the
+ * array-to-{@code HashMap} transition in {@code ListHashMap.put}.
Review Comment:
The class Javadoc still says the default `newMetaDataMap()` returns a raw
`ListHashMap` that is not thread-safe, but in this PR the default is changed to
`Collections.synchronizedMap(new ListHashMap())`. Please update the Javadoc to
reflect the new default (e.g., mention the prior behavior and that the wrapper
makes per-entry operations thread-safe).
```suggestion
* shared AST node. The default {@code newMetaDataMap()} now returns
* {@code Collections.synchronizedMap(new ListHashMap())}; previously it
returned a raw
* {@code ListHashMap}. The synchronized wrapper makes individual map
operations
* thread-safe, and this stress test helps guard against regressions for
concurrent
* compiles sharing a {@link ClassNode} (e.g. built-in annotation nodes
cached by
* {@link ClassHelper}), including the array-to-{@code HashMap} transition in
* {@code ListHashMap.put}.
```
> 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)
