You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by "paulk-asert (via GitHub)" <gi...@apache.org> on 2026/04/26 09:08:22 UTC
[PR] GROOVY-11966: AnnotationNode.isTargetAllowed introduces concurrent wr… [groovy]
paulk-asert opened a new pull request, #2492:
URL: https://github.com/apache/groovy/pull/2492
…ite to shared ListHashMap
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11966: AnnotationNode.isTargetAllowed introduces concurrent wr… [groovy]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2492:
URL: https://github.com/apache/groovy/pull/2492#issuecomment-4321733700
## [Codecov](https://app.codecov.io/gh/apache/groovy/pull/2492?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 67.1168%. Comparing base ([`ac71deb`](https://app.codecov.io/gh/apache/groovy/commit/ac71deb69d3193d07052fcaa57e3a442baf4e244?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`e8bbc97`](https://app.codecov.io/gh/apache/groovy/commit/e8bbc97b86f2ea40efdffcb03f3c07e23e0c6754?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/2492?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2492 +/- ##
==================================================
+ Coverage 67.1130% 67.1168% +0.0038%
Complexity 31605 31605
==================================================
Files 1451 1451
Lines 122556 122567 +11
Branches 22006 22006
==================================================
+ Hits 82251 82263 +12
- Misses 33220 33221 +1
+ Partials 7085 7083 -2
```
| [Files with missing lines](https://app.codecov.io/gh/apache/groovy/pull/2492?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...a/org/codehaus/groovy/ast/NodeMetaDataHandler.java](https://app.codecov.io/gh/apache/groovy/pull/2492?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fast%2FNodeMetaDataHandler.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2FzdC9Ob2RlTWV0YURhdGFIYW5kbGVyLmphdmE=) | `92.4528% <100.0000%> (+1.9766%)` | :arrow_up: |
... and [23 files with indirect coverage changes](https://app.codecov.io/gh/apache/groovy/pull/2492/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11966: AnnotationNode.isTargetAllowed introduces concurrent wr… [groovy]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11966: AnnotationNode.isTargetAllowed introduces concurrent wr… [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert merged PR #2492:
URL: https://github.com/apache/groovy/pull/2492
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-11966: AnnotationNode.isTargetAllowed introduces concurrent wr… [groovy]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2492:
URL: https://github.com/apache/groovy/pull/2492#discussion_r3144678223
##########
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 synchronization, 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}.
Review Comment:
The class Javadoc still says the default `newMetaDataMap()` returns a raw `ListHashMap` that is not thread-safe, but in this PR the default is changed to `Collections.synchronizedMap(new ListHashMap())`. Please update the Javadoc to reflect the new default (e.g., mention the prior behavior and that the wrapper makes per-entry operations thread-safe).
```suggestion
* shared AST node. The default {@code newMetaDataMap()} now returns
* {@code Collections.synchronizedMap(new ListHashMap())}; previously it returned a raw
* {@code ListHashMap}. The synchronized wrapper makes individual map operations
* thread-safe, and this stress test helps guard against regressions for concurrent
* compiles sharing a {@link ClassNode} (e.g. built-in annotation nodes cached by
* {@link ClassHelper}), including the array-to-{@code HashMap} transition in
* {@code ListHashMap.put}.
```
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org