[
https://issues.apache.org/jira/browse/GROOVY-11966?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18076344#comment-18076344
]
ASF GitHub Bot commented on GROOVY-11966:
-----------------------------------------
Copilot commented on code in PR #2492:
URL: https://github.com/apache/groovy/pull/2492#discussion_r3143662245
##########
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 synchronisation, 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}.
+ */
+public class NodeMetaDataHandlerStressTest {
+
+ private static final int THREADS = 16;
+ private static final int ITERATIONS = 5_000;
+ private static final int KEY_SPACE = 4;
+
+ @Test
+ public void testConcurrentAccessOnSharedNode() throws Exception {
+ // ClassHelper.makeCached returns a process-wide shared ClassNode;
this is
+ // the path that built-in annotation ClassNodes reach in real
compilations.
+ // Use a unique target class so other tests in the same JVM don't
interfere.
+ ClassNode shared = ClassHelper.makeCached(SharedTarget.class);
+
+ CyclicBarrier start = new CyclicBarrier(THREADS);
+ List<Throwable> errors = new CopyOnWriteArrayList<>();
+ ExecutorService pool = Executors.newFixedThreadPool(THREADS);
+ try {
+ for (int t = 0; t < THREADS; t++) {
+ final int threadId = t;
+ pool.submit(() -> {
+ ThreadUtils.await(start);
+ try {
Review Comment:
The stress test submits tasks via `ExecutorService.submit(...)` but never
inspects the returned `Future`s. Any exception that escapes the inner try/catch
(e.g. from `ThreadUtils.await(start)` before the try block, or other unexpected
failures) will be captured by the `Future` and can make the test pass silently.
Consider either moving the barrier await inside the try/catch that records
`errors`, or collecting the `Future`s and calling `get()`
(recording/propagating `ExecutionException`s) after `awaitTermination`.
```suggestion
try {
ThreadUtils.await(start);
```
##########
src/main/java/org/codehaus/groovy/ast/NodeMetaDataHandler.java:
##########
@@ -22,11 +22,18 @@
import org.codehaus.groovy.util.ListHashMap;
import java.util.Collections;
+import java.util.HashMap;
import java.util.Map;
import java.util.function.Function;
/**
* An interface to mark a node being able to handle metadata.
+ * <p>
+ * The default {@link #newMetaDataMap()} returns a {@link ListHashMap}, which
is
+ * not thread-safe. Default methods here serialise map access on {@code this}
so
Review Comment:
Javadoc uses "serialise" here, but the rest of the codebase appears to
consistently use American English (e.g. "serialize"). Consider changing this to
"serialize" for consistency.
```suggestion
* not thread-safe. Default methods here serialize map access on {@code
this} so
```
> 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)
