Copilot commented on code in PR #15960: URL: https://github.com/apache/grails-core/pull/15960#discussion_r3560673437
########## grails-async/core/src/main/groovy/org/grails/async/factory/future/VirtualThreadPromiseFactory.groovy: ########## @@ -0,0 +1,111 @@ +/* + * 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 + * + * https://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.grails.async.factory.future + +import java.util.concurrent.Callable +import java.util.concurrent.ExecutorService +import java.util.concurrent.Executors +import java.util.concurrent.TimeUnit + +import groovy.transform.AutoFinal +import groovy.transform.CompileStatic + +import jakarta.annotation.PreDestroy + +import grails.async.Promise +import grails.async.PromiseList +import grails.async.factory.AbstractPromiseFactory +import org.grails.async.factory.BoundPromise + +/** + * PromiseFactory implementation backed by Java virtual threads. + * + * @since 8.1 + */ +@AutoFinal +@CompileStatic +class VirtualThreadPromiseFactory extends AbstractPromiseFactory implements Closeable { + + private final ExecutorService executorService = Executors.newVirtualThreadPerTaskExecutor() + + @Override + <T> Promise<T> createPromise(Class<T> returnType) { + return new BoundPromise<T>(null) + } + + @Override + Promise<Object> createPromise() { + return new BoundPromise<Object>(null) + } + + @Override + <T> Promise<T> createPromise(Closure<T>... closures) { + if (closures.length == 1) { + Closure<T> decoratedCallable = applyDecorators(closures[0], null) + FutureTaskPromise<T> promise = new FutureTaskPromise<T>(this, decoratedCallable as Callable<T>) + executorService.execute(promise) + return promise + } + + PromiseList<T> list = new PromiseList<>() + for (Closure<T> closure : closures) { + list.add(closure) + } + return list as Promise<T> + } + + @Override + <T> List<T> waitAll(List<Promise<T>> promises) { + return promises.collect { Promise<T> promise -> promise.get() } + } + + @Override + <T> List<T> waitAll(List<Promise<T>> promises, long timeout, TimeUnit units) { + return promises.collect { Promise<T> promise -> promise.get(timeout, units) } + } + + @Override + <T> Promise<List<T>> onComplete(List<Promise<T>> promises, Closure<T> callable) { + return createPromise({ + List<T> values = waitAll(promises) + callable.call(values) + } as Closure<List<T>>) + } + + @Override + <T> Promise<List<T>> onError(List<Promise<T>> promises, Closure<?> callable) { + return createPromise({ + try { + waitAll(promises) + return null + } + catch (Throwable e) { + callable.call(e) + return e + } + } as Closure<List<T>>) + } Review Comment: `onComplete`/`onError` currently cast the task closure to `Closure<List<T>>` but the closure returns `callable.call(values)`, `null`, or a `Throwable`. Under `@CompileStatic` this can lead to runtime `ClassCastException` when `FutureTaskPromise.get()` coerces the result to `List<T>` (e.g. returning `Throwable` from `onError`). Use a raw/`Object`-typed `FutureTaskPromise` (or submit via the executor) and only cast the returned Promise at the API boundary, similar to `CachedThreadPoolPromiseFactory`. ########## grails-async/core/src/main/groovy/org/grails/async/factory/future/VirtualThreadPromiseFactory.groovy: ########## @@ -0,0 +1,111 @@ +/* + * 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 + * + * https://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.grails.async.factory.future + +import java.util.concurrent.Callable +import java.util.concurrent.ExecutorService +import java.util.concurrent.Executors +import java.util.concurrent.TimeUnit + +import groovy.transform.AutoFinal +import groovy.transform.CompileStatic + +import jakarta.annotation.PreDestroy + +import grails.async.Promise +import grails.async.PromiseList +import grails.async.factory.AbstractPromiseFactory +import org.grails.async.factory.BoundPromise + +/** + * PromiseFactory implementation backed by Java virtual threads. + * + * @since 8.1 + */ +@AutoFinal +@CompileStatic +class VirtualThreadPromiseFactory extends AbstractPromiseFactory implements Closeable { + + private final ExecutorService executorService = Executors.newVirtualThreadPerTaskExecutor() + + @Override + <T> Promise<T> createPromise(Class<T> returnType) { + return new BoundPromise<T>(null) + } + + @Override + Promise<Object> createPromise() { + return new BoundPromise<Object>(null) + } + + @Override + <T> Promise<T> createPromise(Closure<T>... closures) { + if (closures.length == 1) { + Closure<T> decoratedCallable = applyDecorators(closures[0], null) + FutureTaskPromise<T> promise = new FutureTaskPromise<T>(this, decoratedCallable as Callable<T>) + executorService.execute(promise) + return promise + } + + PromiseList<T> list = new PromiseList<>() + for (Closure<T> closure : closures) { + list.add(closure) + } + return list as Promise<T> + } + + @Override + <T> List<T> waitAll(List<Promise<T>> promises) { + return promises.collect { Promise<T> promise -> promise.get() } + } + + @Override + <T> List<T> waitAll(List<Promise<T>> promises, long timeout, TimeUnit units) { + return promises.collect { Promise<T> promise -> promise.get(timeout, units) } + } + + @Override + <T> Promise<List<T>> onComplete(List<Promise<T>> promises, Closure<T> callable) { + return createPromise({ + List<T> values = waitAll(promises) + callable.call(values) + } as Closure<List<T>>) + } + + @Override + <T> Promise<List<T>> onError(List<Promise<T>> promises, Closure<?> callable) { + return createPromise({ + try { + waitAll(promises) + return null + } + catch (Throwable e) { + callable.call(e) + return e + } + } as Closure<List<T>>) + } + + @Override + @PreDestroy + void close() { + executorService.shutdown() + } Review Comment: `close()` should be idempotent like `CachedThreadPoolPromiseFactory.close()`; guarding avoids redundant shutdown calls and keeps behavior consistent across promise factories. ########## grails-async/core/src/main/groovy/org/grails/async/factory/future/VirtualThreadPromiseFactory.groovy: ########## @@ -0,0 +1,111 @@ +/* + * 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 + * + * https://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.grails.async.factory.future + +import java.util.concurrent.Callable +import java.util.concurrent.ExecutorService +import java.util.concurrent.Executors +import java.util.concurrent.TimeUnit + +import groovy.transform.AutoFinal +import groovy.transform.CompileStatic + +import jakarta.annotation.PreDestroy + +import grails.async.Promise +import grails.async.PromiseList +import grails.async.factory.AbstractPromiseFactory +import org.grails.async.factory.BoundPromise + +/** + * PromiseFactory implementation backed by Java virtual threads. + * + * @since 8.1 Review Comment: The `@since 8.1` tag doesn’t match the current branch/version line (`8.0.x`, 8.0.0-SNAPSHOT). This should reflect the first Grails version that will actually ship this class (likely 8.0.x). ########## grails-doc/src/en/guide/async/asyncPromises.adoc: ########## @@ -93,6 +93,10 @@ def result = p.get(1,MINUTES) By default, the `Promises` static methods use an instance of `PromiseFactory`. This `PromiseFactory` interface has various implementations. The default implementation is link:{api}org/grails/async/factory/future/CachedThreadPoolPromiseFactory.html[CachedThreadPoolPromiseFactory] which uses a thread pool that will create threads as needed (the same as `java.util.concurrent.Executors.newCachedThreadPool()`) +Grails 8.1 also includes an opt-in Java 21 virtual-thread seed implementation, `org.grails.async.factory.future.VirtualThreadPromiseFactory`. Review Comment: This guide is on the `8.0.x` branch (projectVersion is 8.0.0-SNAPSHOT); stating "Grails 8.1" here is likely to become incorrect/out-of-date once merged. Prefer a version-neutral statement ("Grails 8") or align to 8.0.x. ########## grails-async/core/src/test/groovy/grails/async/VirtualThreadPromiseFactorySpec.groovy: ########## @@ -0,0 +1,53 @@ +/* + * 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 + * + * https://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 grails.async + +import org.grails.async.factory.PromiseFactoryBuilder +import org.grails.async.factory.future.VirtualThreadPromiseFactory +import spock.lang.Specification + +class VirtualThreadPromiseFactorySpec extends Specification { + + def cleanup() { + System.clearProperty('grails.async.promiseFactory') + Promises.promiseFactory = null + } + + void 'builder can opt in to virtual thread promise factory'() { + given: + System.setProperty('grails.async.promiseFactory', 'virtual-thread') + + expect: + PromiseFactoryBuilder.build() instanceof VirtualThreadPromiseFactory + } Review Comment: This spec mutates global state (`System` properties and `Promises.promiseFactory`) but only clears them. For test isolation (especially with parallel forks), snapshot and restore the previous values, and ensure the factory built by `PromiseFactoryBuilder.build()` is closed to avoid leaking an ExecutorService into the JVM running the test suite. -- 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]
