GROOVY-11966: AnnotationNode.isTargetAllowed introduces concurrent write to shared ListHashMap

Author: paulk-asert

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());

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<>();

src/main/java/org/codehaus/groovy/ast/NodeMetaDataHandler.java

@@ -22,11 +22,20 @@
 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 @@ default <T> T getNodeMetaData(Object key) {
     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 @@ default void copyNodeMetaData(NodeMetaDataHandler other) {
         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 @@ default void setNodeMetaData(Object key, Object value) {
     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 @@ default void removeNodeMetaData(Object key) {
     }
 
     /**
-     * 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 @@ default void removeNodeMetaData(Object key) {
         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);

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 { }
+}