You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@groovy.apache.org by pa...@apache.org on 2026/05/01 18:58:57 UTC
(groovy) branch master updated: GROOVY-11966: AnnotationNode.isTargetAllowed introduces concurrent write to shared ListHashMap
This is an automated email from the ASF dual-hosted git repository.
paulk-asert pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/groovy.git
The following commit(s) were added to refs/heads/master by this push:
new 8dde1c8413 GROOVY-11966: AnnotationNode.isTargetAllowed introduces concurrent write to shared ListHashMap
8dde1c8413 is described below
commit 8dde1c84134ef6fdeecf26b5cbb5183d5aab4dac
Author: Paul King <pa...@asert.com.au>
AuthorDate: Sun Apr 26 19:07:56 2026 +1000
GROOVY-11966: AnnotationNode.isTargetAllowed introduces concurrent write to shared ListHashMap
---
src/main/java/org/codehaus/groovy/ast/ASTNode.java | 2 +-
.../java/org/codehaus/groovy/ast/CompileUnit.java | 2 +-
.../codehaus/groovy/ast/NodeMetaDataHandler.java | 83 ++++++++++++-----
.../groovy/ast/NodeMetaDataHandlerStressTest.java | 103 +++++++++++++++++++++
4 files changed, 165 insertions(+), 25 deletions(-)
diff --git a/src/main/java/org/codehaus/groovy/ast/ASTNode.java b/src/main/java/org/codehaus/groovy/ast/ASTNode.java
index 3de425dc5d..466b1f142f 100644
--- a/src/main/java/org/codehaus/groovy/ast/ASTNode.java
+++ b/src/main/java/org/codehaus/groovy/ast/ASTNode.java
@@ -46,7 +46,7 @@ public class ASTNode implements NodeMetaDataHandler {
private int lastLineNumber = -1;
private int lastColumnNumber = -1;
- private Map<?, ?> metaDataMap;
+ private volatile Map<?, ?> metaDataMap;
public void visit(final GroovyCodeVisitor visitor) {
throw new RuntimeException("No visit() method implemented for class: " + getClass().getName());
diff --git a/src/main/java/org/codehaus/groovy/ast/CompileUnit.java b/src/main/java/org/codehaus/groovy/ast/CompileUnit.java
index 970dfe2f52..524bc6c227 100644
--- a/src/main/java/org/codehaus/groovy/ast/CompileUnit.java
+++ b/src/main/java/org/codehaus/groovy/ast/CompileUnit.java
@@ -44,7 +44,7 @@ public class CompileUnit implements NodeMetaDataHandler {
private final CompilerConfiguration config;
private final GroovyClassLoader loader;
private final CodeSource codeSource;
- private Map<?, ?> metaDataMap;
+ private volatile Map<?, ?> metaDataMap;
private final List<ModuleNode> modules = new ArrayList<>();
private final Map<String, ClassNode> classes = new LinkedHashMap<>();
diff --git a/src/main/java/org/codehaus/groovy/ast/NodeMetaDataHandler.java b/src/main/java/org/codehaus/groovy/ast/NodeMetaDataHandler.java
index ddb485d4f1..d039c3f576 100644
--- a/src/main/java/org/codehaus/groovy/ast/NodeMetaDataHandler.java
+++ b/src/main/java/org/codehaus/groovy/ast/NodeMetaDataHandler.java
@@ -22,11 +22,20 @@ import org.codehaus.groovy.GroovyBugError;
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} wrapped in
+ * {@link Collections#synchronizedMap}, so concurrent compiles sharing an AST node
+ * (e.g. built-in annotation {@code ClassNode}s cached by {@code ClassHelper})
+ * cannot trip an {@code ArrayIndexOutOfBoundsException} during the
+ * array-to-{@link HashMap} transition in {@code ListHashMap.put}. Implementers
+ * that store the map in a field should declare it {@code volatile} so the
+ * unsynchronized fast-path read in the default methods sees publish-time writes.
*
* @since 3.0.0
*/
@@ -57,12 +66,7 @@ public interface NodeMetaDataHandler {
default <T> T getNodeMetaData(Object key, Function<?, ? extends T> valFn) {
if (key == null) throw new GroovyBugError("Tried to get/set meta data with null key on " + this + ".");
- Map metaDataMap = this.getMetaDataMap();
- if (metaDataMap == null) {
- metaDataMap = this.newMetaDataMap();
- this.setMetaDataMap(metaDataMap);
- }
- return (T) metaDataMap.computeIfAbsent(key, valFn);
+ return (T) getOrCreateMetaDataMap().computeIfAbsent(key, valFn);
}
/**
@@ -75,13 +79,13 @@ public interface NodeMetaDataHandler {
if (otherMetaDataMap == null) {
return;
}
- Map metaDataMap = this.getMetaDataMap();
- if (metaDataMap == null) {
- metaDataMap = this.newMetaDataMap();
- this.setMetaDataMap(metaDataMap);
+ // snapshot under the source map's mutex to honour the synchronizedMap iteration contract
+ Map snapshot;
+ synchronized (otherMetaDataMap) {
+ if (otherMetaDataMap.isEmpty()) return;
+ snapshot = new HashMap<>(otherMetaDataMap);
}
-
- metaDataMap.putAll(otherMetaDataMap);
+ getOrCreateMetaDataMap().putAll(snapshot);
}
/**
@@ -107,15 +111,11 @@ public interface NodeMetaDataHandler {
default Object putNodeMetaData(Object key, Object value) {
if (key == null) throw new GroovyBugError("Tried to set meta data with null key on " + this + ".");
- Map metaDataMap = this.getMetaDataMap();
- if (metaDataMap == null) {
- if (value == null) return null;
- metaDataMap = newMetaDataMap();
- this.setMetaDataMap(metaDataMap);
- } else if (value == null) {
- return metaDataMap.remove(key);
+ if (value == null) {
+ Map metaDataMap = this.getMetaDataMap();
+ return metaDataMap == null ? null : metaDataMap.remove(key);
}
- return metaDataMap.put(key, value);
+ return getOrCreateMetaDataMap().put(key, value);
}
/**
@@ -135,7 +135,7 @@ public interface NodeMetaDataHandler {
}
/**
- * Returns an unmodifiable view of the current node metadata.
+ * Returns an unmodifiable snapshot of the current node metadata.
*
* @return the node metadata. Always not null.
*/
@@ -144,18 +144,55 @@ public interface NodeMetaDataHandler {
if (metaDataMap == null) {
return Collections.emptyMap();
}
- return Collections.unmodifiableMap(metaDataMap);
+ // snapshot under the map's mutex to honour the synchronizedMap iteration contract
+ synchronized (metaDataMap) {
+ return Collections.unmodifiableMap(new HashMap<>(metaDataMap));
+ }
+ }
+
+ /**
+ * Returns the existing metadata map, creating one via {@link #newMetaDataMap()}
+ * on first use. Lazy creation is guarded by a brief lock on {@code this} so
+ * concurrent first-callers agree on a single map; subsequent callers see the
+ * map via the (volatile) field read and skip the lock entirely.
+ */
+ private Map getOrCreateMetaDataMap() {
+ Map metaDataMap = this.getMetaDataMap();
+ if (metaDataMap != null) return metaDataMap;
+ synchronized (this) {
+ metaDataMap = this.getMetaDataMap();
+ if (metaDataMap == null) {
+ metaDataMap = this.newMetaDataMap();
+ this.setMetaDataMap(metaDataMap);
+ }
+ return metaDataMap;
+ }
}
//--------------------------------------------------------------------------
+ /**
+ * Returns the underlying metadata map. The map returned by the default
+ * {@link #newMetaDataMap()} is internally synchronized, so individual
+ * {@code get}/{@code put}/{@code remove} calls are thread-safe; however,
+ * per the {@link Collections#synchronizedMap} contract, iteration over the
+ * returned map (or any of its {@code keySet}, {@code values}, or
+ * {@code entrySet} views) must be done inside a
+ * {@code synchronized (map) { ... }} block to avoid
+ * {@code ConcurrentModificationException}.
+ */
Map<?, ?> getMetaDataMap();
/**
+ * Creates the backing metadata map. The default returns a {@link ListHashMap}
+ * wrapped in {@link Collections#synchronizedMap} for thread-safe per-entry
+ * access; subclasses may override to supply an alternative map (e.g. for
+ * different memory/concurrency trade-offs).
+ *
* @since 5.0.0
*/
default Map<?, ?> newMetaDataMap() {
- return new ListHashMap();
+ return Collections.synchronizedMap(new ListHashMap());
}
void setMetaDataMap(Map<?, ?> metaDataMap);
diff --git a/subprojects/stress/src/stressTest/java/org/codehaus/groovy/ast/NodeMetaDataHandlerStressTest.java b/subprojects/stress/src/stressTest/java/org/codehaus/groovy/ast/NodeMetaDataHandlerStressTest.java
new file mode 100644
index 0000000000..068a309b35
--- /dev/null
+++ b/subprojects/stress/src/stressTest/java/org/codehaus/groovy/ast/NodeMetaDataHandlerStressTest.java
@@ -0,0 +1,103 @@
+/*
+ * 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 underlying {@code ListHashMap} is not thread-safe, so without
+ * external protection 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}. The default
+ * {@code newMetaDataMap()} now wraps the {@code ListHashMap} in
+ * {@link java.util.Collections#synchronizedMap}, making individual {@code get}/{@code put}/
+ * {@code remove}/{@code computeIfAbsent} operations thread-safe; this test verifies that
+ * guarantee under contention.
+ */
+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(() -> {
+ try {
+ ThreadUtils.await(start);
+ for (int i = 0; i < ITERATIONS; i++) {
+ final int iter = i;
+ String key = "k" + (i % KEY_SPACE);
+ String otherKey = "k" + ((i + 1) % KEY_SPACE);
+
+ // factory variant — the path used by AnnotationNode.isTargetAllowed
+ shared.getNodeMetaData(key, k -> "v-" + threadId + "-" + iter);
+ // plain read
+ shared.getNodeMetaData(otherKey);
+ // explicit put / remove to force the array<->HashMap transition
+ if (i % 7 == 0) shared.putNodeMetaData(key, "p-" + threadId + "-" + i);
+ if (i % 11 == 0) shared.removeNodeMetaData(key);
+ }
+ } catch (Throwable th) {
+ errors.add(th);
+ }
+ });
+ }
+ pool.shutdown();
+ if (!pool.awaitTermination(60, TimeUnit.SECONDS)) {
+ throw new AssertionError("stress test did not complete within 60s");
+ }
+ } finally {
+ pool.shutdownNow();
+ }
+
+ if (!errors.isEmpty()) {
+ AssertionError ae = new AssertionError(
+ "concurrent NodeMetaDataHandler access produced " + errors.size() + " errors; first: " + errors.get(0));
+ ae.initCause(errors.get(0));
+ throw ae;
+ }
+ assertEquals(0, errors.size());
+ }
+
+ /** Dedicated target class so we don't poison metadata on a shared standard ClassNode. */
+ private static final class SharedTarget { }
+}