Copilot commented on code in PR #2386: URL: https://github.com/apache/groovy/pull/2386#discussion_r2868678149
########## src/test/groovy/org/codehaus/groovy/transform/AsyncVirtualThreadTest.groovy: ########## @@ -0,0 +1,1138 @@ +/* + * 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.transform + +import org.junit.jupiter.api.Test + +import static groovy.test.GroovyAssert.assertScript + +/** + * Tests for virtual thread integration, executor configuration, exception + * handling consistency across async methods/closures/lambdas, and coverage + * improvements for the async/await feature. + * + * @since 6.0.0 + */ +final class AsyncVirtualThreadTest { + + // ---- Virtual thread detection and usage ---- + + @Test + void testVirtualThreadsAvailable() { + assertScript ''' + import static groovy.concurrent.AsyncUtils.* + // JDK 21+ should have virtual threads + def jdkVersion = Runtime.version().feature() + if (jdkVersion >= 21) { + assert isVirtualThreadsAvailable() + } + ''' + } + + @Test + void testAsyncMethodRunsOnVirtualThread() { + assertScript ''' + import groovy.transform.Async + import static groovy.concurrent.AsyncUtils.* + + class VTService { + async checkThread() { + return Thread.currentThread().isVirtual() + } + } + + if (isVirtualThreadsAvailable()) { + def service = new VTService() + def result = service.checkThread().get() + assert result == true + } + ''' + } + + @Test + void testAsyncClosureRunsOnVirtualThread() { + assertScript ''' + import static groovy.concurrent.AsyncUtils.* + + if (isVirtualThreadsAvailable()) { + def asyncVirtual = async { Thread.currentThread().isVirtual() } + def awaitable = asyncVirtual() + assert await(awaitable) == true + } + ''' + } + + @Test + void testAsyncLambdaRunsOnVirtualThread() { + assertScript ''' + import static groovy.concurrent.AsyncUtils.* + + if (isVirtualThreadsAvailable()) { + def asyncFn = async { x -> Thread.currentThread().isVirtual() } + def result = await(asyncFn(42)) + assert result == true + } + ''' + } + + @Test + void testForAwaitRunsOnVirtualThread() { + assertScript ''' + import groovy.transform.Async + import static groovy.concurrent.AsyncUtils.* + import groovy.concurrent.AsyncStream + + class VTGenerator { + async generate() { + yield return Thread.currentThread().isVirtual() + } + } + + if (isVirtualThreadsAvailable()) { + def gen = new VTGenerator() + def stream = gen.generate() + def results = [] + for await (item in stream) { + results << item + } + assert results == [true] + } + ''' + } + + @Test + void testHighConcurrencyWithVirtualThreads() { + assertScript ''' + import groovy.transform.Async + import static groovy.concurrent.AsyncUtils.* + + class HighConcurrency { + async compute(int n) { + Thread.sleep(10) + return n * 2 + } + } + + if (isVirtualThreadsAvailable()) { + def svc = new HighConcurrency() + // Launch 1000 concurrent tasks — trivial with virtual threads + def awaitables = (1..1000).collect { svc.compute(it) } + def results = awaitAll(awaitables as Object[]) + assert results.size() == 1000 + assert results[0] == 2 + assert results[999] == 2000 + } + ''' + } + + // ---- Executor configuration ---- + + @Test + void testCustomExecutorOverride() { + assertScript ''' + import static groovy.concurrent.AsyncUtils.* + import java.util.concurrent.Executors + import java.util.concurrent.atomic.AtomicReference + + def savedExecutor = getExecutor() + try { + def customPool = Executors.newFixedThreadPool(2, { r -> + def t = new Thread(r) + t.setName("custom-async-" + t.getId()) + t + }) + setExecutor(customPool) + + def asyncName = async { + Thread.currentThread().getName() + } + def awaitable = asyncName() + def threadName = await(awaitable) + assert threadName.startsWith("custom-async-") + } finally { + setExecutor(savedExecutor) + } + ''' + } + + @Test + void testSetExecutorNullResetsToDefault() { + assertScript ''' + import static groovy.concurrent.AsyncUtils.* + import java.util.concurrent.Executors + + def originalExecutor = getExecutor() + // Set a custom executor + setExecutor(Executors.newSingleThreadExecutor()) + assert getExecutor() != originalExecutor + // Reset to null — should restore default + setExecutor(null) Review Comment: This test sets a newSingleThreadExecutor() as the global async executor but never shuts that executor down. Even after setExecutor(null), the executor thread remains alive and can keep the JVM running. Please keep a reference to the created executor and shut it down (or create it with daemon threads). ########## src/test/groovy/org/codehaus/groovy/transform/AsyncVirtualThreadTest.groovy: ########## @@ -0,0 +1,1138 @@ +/* + * 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.transform + +import org.junit.jupiter.api.Test + +import static groovy.test.GroovyAssert.assertScript + +/** + * Tests for virtual thread integration, executor configuration, exception + * handling consistency across async methods/closures/lambdas, and coverage + * improvements for the async/await feature. + * + * @since 6.0.0 + */ +final class AsyncVirtualThreadTest { + + // ---- Virtual thread detection and usage ---- + + @Test + void testVirtualThreadsAvailable() { + assertScript ''' + import static groovy.concurrent.AsyncUtils.* + // JDK 21+ should have virtual threads + def jdkVersion = Runtime.version().feature() + if (jdkVersion >= 21) { + assert isVirtualThreadsAvailable() + } + ''' + } + + @Test + void testAsyncMethodRunsOnVirtualThread() { + assertScript ''' + import groovy.transform.Async + import static groovy.concurrent.AsyncUtils.* + + class VTService { + async checkThread() { + return Thread.currentThread().isVirtual() + } + } + + if (isVirtualThreadsAvailable()) { + def service = new VTService() + def result = service.checkThread().get() + assert result == true + } + ''' + } + + @Test + void testAsyncClosureRunsOnVirtualThread() { + assertScript ''' + import static groovy.concurrent.AsyncUtils.* + + if (isVirtualThreadsAvailable()) { + def asyncVirtual = async { Thread.currentThread().isVirtual() } + def awaitable = asyncVirtual() + assert await(awaitable) == true + } + ''' + } + + @Test + void testAsyncLambdaRunsOnVirtualThread() { + assertScript ''' + import static groovy.concurrent.AsyncUtils.* + + if (isVirtualThreadsAvailable()) { + def asyncFn = async { x -> Thread.currentThread().isVirtual() } + def result = await(asyncFn(42)) + assert result == true + } + ''' + } + + @Test + void testForAwaitRunsOnVirtualThread() { + assertScript ''' + import groovy.transform.Async + import static groovy.concurrent.AsyncUtils.* + import groovy.concurrent.AsyncStream + + class VTGenerator { + async generate() { + yield return Thread.currentThread().isVirtual() + } + } + + if (isVirtualThreadsAvailable()) { + def gen = new VTGenerator() + def stream = gen.generate() + def results = [] + for await (item in stream) { + results << item + } + assert results == [true] + } + ''' + } + + @Test + void testHighConcurrencyWithVirtualThreads() { + assertScript ''' + import groovy.transform.Async + import static groovy.concurrent.AsyncUtils.* + + class HighConcurrency { + async compute(int n) { + Thread.sleep(10) + return n * 2 + } + } + + if (isVirtualThreadsAvailable()) { + def svc = new HighConcurrency() + // Launch 1000 concurrent tasks — trivial with virtual threads + def awaitables = (1..1000).collect { svc.compute(it) } + def results = awaitAll(awaitables as Object[]) + assert results.size() == 1000 + assert results[0] == 2 + assert results[999] == 2000 + } + ''' + } + + // ---- Executor configuration ---- + + @Test + void testCustomExecutorOverride() { + assertScript ''' + import static groovy.concurrent.AsyncUtils.* + import java.util.concurrent.Executors + import java.util.concurrent.atomic.AtomicReference Review Comment: Unused import: `java.util.concurrent.atomic.AtomicReference` isn't referenced in this script. Removing it will keep the test focused and avoid lint noise. ```suggestion ``` ########## src/main/java/org/codehaus/groovy/transform/AsyncASTTransformation.java: ########## @@ -0,0 +1,255 @@ +/* + * 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.transform; + +import groovy.concurrent.AsyncStream; +import groovy.concurrent.Awaitable; +import groovy.transform.Async; +import org.apache.groovy.runtime.async.AsyncSupport; +import org.codehaus.groovy.ast.ASTNode; +import org.codehaus.groovy.ast.AnnotatedNode; +import org.codehaus.groovy.ast.AnnotationNode; +import org.codehaus.groovy.ast.ClassCodeExpressionTransformer; +import org.codehaus.groovy.ast.ClassHelper; +import org.codehaus.groovy.ast.ClassNode; +import org.codehaus.groovy.ast.FieldNode; +import org.codehaus.groovy.ast.GenericsType; +import org.codehaus.groovy.ast.MethodNode; +import org.codehaus.groovy.ast.Parameter; +import org.codehaus.groovy.ast.VariableScope; +import org.codehaus.groovy.ast.expr.ArgumentListExpression; +import org.codehaus.groovy.ast.expr.ClosureExpression; +import org.codehaus.groovy.ast.expr.Expression; +import org.codehaus.groovy.ast.expr.MethodCallExpression; +import org.codehaus.groovy.ast.expr.StaticMethodCallExpression; +import org.codehaus.groovy.ast.stmt.Statement; +import org.codehaus.groovy.control.CompilePhase; +import org.codehaus.groovy.control.SourceUnit; + +import static org.codehaus.groovy.ast.tools.GeneralUtils.args; +import static org.codehaus.groovy.ast.tools.GeneralUtils.block; +import static org.codehaus.groovy.ast.tools.GeneralUtils.callX; +import static org.codehaus.groovy.ast.tools.GeneralUtils.returnS; +import static org.codehaus.groovy.ast.tools.GeneralUtils.varX; +import static org.codehaus.groovy.ast.tools.GenericsUtils.makeClassSafeWithGenerics; + +/** + * Handles code generation for the {@link Async @Async} annotation. + * <p> + * Transforms the annotated method so that: + * <ol> + * <li>{@code await(expr)} calls within the method body are redirected to + * {@link AsyncSupport#await(Object) AsyncSupport.await()}</li> + * <li>The method body is executed asynchronously via + * {@link AsyncSupport#executeAsync AsyncSupport.executeAsync} + * (or {@link AsyncSupport#executeAsyncVoid AsyncSupport.executeAsyncVoid} + * for {@code void} methods)</li> + * <li>Generator methods (containing {@code yield return}) are transformed to + * use {@link AsyncSupport#generateAsyncStream AsyncSupport.generateAsyncStream}, + * returning an {@link AsyncStream}{@code <T>}</li> + * <li>The return type becomes {@link Awaitable}{@code <T>} + * (or {@link AsyncStream}{@code <T>} for generators)</li> + * </ol> + * <p> + * Yield-return detection and rewriting are delegated to + * {@link AsyncTransformHelper}, which is shared with the ANTLR4 parser's + * {@code async { ... }} closure handling to avoid code duplication. + * <p> + * This transformation runs during the {@link CompilePhase#CANONICALIZATION} + * phase — before type resolution, which allows the modified return types to + * participate in normal type checking. + * + * @see Async + * @see AsyncSupport + * @see AsyncTransformHelper + * @since 6.0.0 + */ +@GroovyASTTransformation(phase = CompilePhase.CANONICALIZATION) +public class AsyncASTTransformation extends AbstractASTTransformation { + + private static final Class<?> MY_CLASS = Async.class; + private static final ClassNode MY_TYPE = ClassHelper.make(MY_CLASS); + private static final String MY_TYPE_NAME = "@" + MY_TYPE.getNameWithoutPackage(); + private static final ClassNode AWAITABLE_TYPE = ClassHelper.make(Awaitable.class); + private static final ClassNode ASYNC_STREAM_TYPE = ClassHelper.make(AsyncStream.class); + + @Override + public void visit(ASTNode[] nodes, SourceUnit source) { + init(nodes, source); + AnnotatedNode parent = (AnnotatedNode) nodes[1]; + AnnotationNode anno = (AnnotationNode) nodes[0]; + if (!MY_TYPE.equals(anno.getClassNode())) return; + + if (!(parent instanceof MethodNode mNode)) return; + + // Validate + if (mNode.isAbstract()) { + addError(MY_TYPE_NAME + " cannot be applied to abstract method '" + mNode.getName() + "'", mNode); + return; + } + if ("<init>".equals(mNode.getName()) || "<clinit>".equals(mNode.getName())) { + addError(MY_TYPE_NAME + " cannot be applied to constructors", mNode); + return; + } + ClassNode originalReturnType = mNode.getReturnType(); + if (AWAITABLE_TYPE.getName().equals(originalReturnType.getName()) + || ASYNC_STREAM_TYPE.getName().equals(originalReturnType.getName()) + || "java.util.concurrent.CompletableFuture".equals(originalReturnType.getName())) { + addError(MY_TYPE_NAME + " cannot be applied to a method that already returns an async type", mNode); + return; + } + Statement originalBody = mNode.getCode(); + if (originalBody == null) return; + + ClassNode cNode = mNode.getDeclaringClass(); + + // Resolve executor expression + String executorFieldName = getMemberStringValue(anno, "executor"); + Expression executorExpr; + if (executorFieldName != null && !executorFieldName.isEmpty()) { + FieldNode field = cNode.getDeclaredField(executorFieldName); + if (field == null) { + addError(MY_TYPE_NAME + ": executor field '" + executorFieldName + + "' not found in class " + cNode.getName(), mNode); + return; + } + if (mNode.isStatic() && !field.isStatic()) { + addError(MY_TYPE_NAME + ": executor field '" + executorFieldName + + "' must be static for static method '" + mNode.getName() + "'", mNode); + return; + } + executorExpr = varX(executorFieldName); + } else { + executorExpr = callX(AsyncTransformHelper.ASYNC_SUPPORT_TYPE, "getExecutor", + new org.codehaus.groovy.ast.expr.ArgumentListExpression()); + } + + // Step 1: Transform await() method calls to AsyncSupport.await() + new AwaitCallTransformer(source).visitMethod(mNode); + + // Step 2: Check if body contains yield return (AsyncSupport.yieldReturn) calls + Statement transformedBody = mNode.getCode(); + boolean hasYieldReturn = AsyncTransformHelper.containsYieldReturn(transformedBody); + + Parameter[] closureParams; + if (hasYieldReturn) { + closureParams = new Parameter[]{ AsyncTransformHelper.createGenParam() }; + } else { + closureParams = Parameter.EMPTY_ARRAY; + } + ClosureExpression closure = new ClosureExpression(closureParams, transformedBody); + VariableScope closureScope = new VariableScope(mNode.getVariableScope()); + for (Parameter p : mNode.getParameters()) { + p.setClosureSharedVariable(true); + closureScope.putReferencedLocalVariable(p); + } + closure.setVariableScope(closureScope); + + if (hasYieldReturn) { + // Transform yieldReturn(expr) → yieldReturn($__asyncGen__, expr) in closure body + AsyncTransformHelper.injectGenParamIntoYieldReturnCalls(transformedBody, closureParams[0]); + + // Async generator: wrap body in AsyncSupport.generateAsyncStream { ... } + Expression genCall = callX(AsyncTransformHelper.ASYNC_SUPPORT_TYPE, + AsyncTransformHelper.GENERATE_ASYNC_STREAM_METHOD, args(closure)); Review Comment: When the annotated method is an async generator (contains `yield return`), the custom executor resolved from `@Async(executor=...)` is ignored and the generator always runs on `AsyncSupport`'s default executor. This makes executor override inconsistent between regular async methods and generator methods; consider adding an executor-aware `generateAsyncStream` overload and using it here. ```suggestion // Async generator: wrap body in AsyncSupport.generateAsyncStream, honoring a custom executor if present ArgumentListExpression genArgs = (executorExpr != null) ? args(executorExpr, closure) : args(closure); Expression genCall = callX(AsyncTransformHelper.ASYNC_SUPPORT_TYPE, AsyncTransformHelper.GENERATE_ASYNC_STREAM_METHOD, genArgs); ``` -- 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]
