diff --git a/src/main/java/org/codehaus/groovy/ast/ASTNode.java b/src/main/java/org/codehaus/groovy/ast/ASTNode.java index 3de425dc5db..466b1f142f4 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 970dfe2f529..524bc6c227d 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 modules = new ArrayList<>(); private final Map 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 ddb485d4f16..d039c3f576c 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.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. + *

+ * 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 getNodeMetaData(Object key) { default T getNodeMetaData(Object key, Function 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); 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 00000000000..068a309b353 --- /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 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 { } +}