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 ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
