From 00f1a3751ebc2ffe744328956c9c108a7486d8c6 Mon Sep 17 00:00:00 2001 From: James Daugherty Date: Wed, 13 May 2026 21:43:28 -0400 Subject: [PATCH 1/2] Async project defensive coding cleanup Defensive coding improvements to the grails-async module: 1. Null-safety guards - AbstractPromiseFactory.createPromise(List, List): return an empty PromiseList when the closures list is null rather than NPE on size(). - FutureTaskPromise.set(T) and setException(Throwable): null-check successCallbacks and failureCallbacks before synchronizing/iterating. 2. Exception handling - DelegateAsyncTransformation.getTransformer(): catch Exception (renamed to ignored) instead of Throwable. Catching Throwable swallows Errors that should propagate (OutOfMemoryError, StackOverflowError, etc.) and is widely considered bad style. 3. Static analysis hints - Add @Override on DelegateAsyncTransformation.visit and on the NoopDelegateAsyncTransactionalMethodTransformer.transformTransactionalMethod overrides. - Use constant-on-left equality (VOID.equals(name) instead of name.equals(VOID)) to avoid NPE if name is null. - Change copyParameters(Parameter[]) to varargs copyParameters(Parameter...) for caller convenience. This change was originally pulled forward into the hibernate7 staging branch (PR #15654 commit e3cad414db) and was flagged by reviewers as unrelated to the Hibernate 7 clone. Extracting it here so it can land on 8.0.x on its own merits as a prerequisite for the Hibernate 7 work. Assisted-by: claude-code:claude-4.7-opus --- .../async/factory/AbstractPromiseFactory.groovy | 3 +++ .../factory/future/FutureTaskPromise.groovy | 16 ++++++++++------ .../internal/DelegateAsyncTransformation.java | 8 +++++--- .../factory/gpars/LoggingPoolFactory.groovy | 2 +- .../PersistenceContextPromiseDecorator.groovy | 2 +- .../async/web/AsyncGrailsWebRequest.groovy | 16 +++++++++------- ...ateAsyncTransactionalMethodTransformer.groovy | 6 ++++-- .../async/AsyncWebRequestPromiseDecorator.groovy | 2 +- .../web/async/spring/PromiseFactoryBean.groovy | 1 - 9 files changed, 34 insertions(+), 22 deletions(-) diff --git a/grails-async/core/src/main/groovy/grails/async/factory/AbstractPromiseFactory.groovy b/grails-async/core/src/main/groovy/grails/async/factory/AbstractPromiseFactory.groovy index caf480a3d22..52105467a94 100644 --- a/grails-async/core/src/main/groovy/grails/async/factory/AbstractPromiseFactory.groovy +++ b/grails-async/core/src/main/groovy/grails/async/factory/AbstractPromiseFactory.groovy @@ -84,6 +84,9 @@ abstract class AbstractPromiseFactory implements PromiseFactory { * @see PromiseFactory#createPromise(java.util.List, java.util.List) */ Promise> createPromise(List> closures, List decorators) { + if (closures == null) { + return new PromiseList() + } List> decoratedClosures = new ArrayList>(closures.size()) for (Closure closure : closures) { decoratedClosures.add(applyDecorators(closure, decorators)) diff --git a/grails-async/core/src/main/groovy/org/grails/async/factory/future/FutureTaskPromise.groovy b/grails-async/core/src/main/groovy/org/grails/async/factory/future/FutureTaskPromise.groovy index 66f72568710..2dcfb58e43b 100644 --- a/grails-async/core/src/main/groovy/org/grails/async/factory/future/FutureTaskPromise.groovy +++ b/grails-async/core/src/main/groovy/org/grails/async/factory/future/FutureTaskPromise.groovy @@ -84,9 +84,11 @@ class FutureTaskPromise extends FutureTask implements Promise { @Override protected void set(T t) { super.set(t) - synchronized (successCallbacks) { - for (FutureTaskChildPromise callback : successCallbacks) { - callback.accept(t) + if (successCallbacks != null) { + synchronized (successCallbacks) { + for (FutureTaskChildPromise callback : successCallbacks) { + callback.accept(t) + } } } } @@ -94,9 +96,11 @@ class FutureTaskPromise extends FutureTask implements Promise { @Override protected void setException(Throwable t) { super.setException(t) - synchronized (failureCallbacks) { - for (FutureTaskChildPromise callback : failureCallbacks) { - callback.accept(t) + if (failureCallbacks != null) { + synchronized (failureCallbacks) { + for (FutureTaskChildPromise callback : failureCallbacks) { + callback.accept(t) + } } } } diff --git a/grails-async/core/src/main/groovy/org/grails/async/transform/internal/DelegateAsyncTransformation.java b/grails-async/core/src/main/groovy/org/grails/async/transform/internal/DelegateAsyncTransformation.java index 1b286533e71..a776e0644dc 100644 --- a/grails-async/core/src/main/groovy/org/grails/async/transform/internal/DelegateAsyncTransformation.java +++ b/grails-async/core/src/main/groovy/org/grails/async/transform/internal/DelegateAsyncTransformation.java @@ -72,6 +72,7 @@ public class DelegateAsyncTransformation implements ASTTransformation, Transform public static final ClassNode GROOVY_OBJECT_CLASS_NODE = new ClassNode(GroovyObjectSupport.class); public static final ClassNode OBJECT_CLASS_NODE = new ClassNode(Object.class); + @Override public void visit(ASTNode[] nodes, SourceUnit source) { if (nodes.length != 2 || !(nodes[0] instanceof AnnotationNode) || !(nodes[1] instanceof AnnotatedNode)) { throw new GroovyBugError("Internal error: expecting [AnnotationNode, AnnotatedNode] but got: " + Arrays.asList(nodes)); @@ -118,7 +119,7 @@ private void applyDelegateAsyncTransform(ClassNode classNode, ClassNode targetAp if (existingMethod == null) { ClassNode promiseNode = ClassHelper.make(Promise.class).getPlainNodeReference(); ClassNode originalReturnType = m.getReturnType(); - if (!originalReturnType.getNameWithoutPackage().equals(VOID)) { + if (!VOID.equals(originalReturnType.getNameWithoutPackage())) { ClassNode returnType; if (ClassHelper.isPrimitiveType(originalReturnType.redirect())) { returnType = ClassHelper.getWrapper(originalReturnType.redirect()); @@ -200,7 +201,7 @@ protected DelegateAsyncTransactionalMethodTransformer lookupAsyncTransactionalMe try { Class transformerClass = getClass().getClassLoader().loadClass("org.grails.async.transform.internal.DefaultDelegateAsyncTransactionalMethodTransformer"); return (DelegateAsyncTransactionalMethodTransformer) transformerClass.getDeclaredConstructor().newInstance(); - } catch (Throwable e) { + } catch (Exception ignored) { // ignore } return new NoopDelegateAsyncTransactionalMethodTransformer(); @@ -214,7 +215,7 @@ private static boolean isCandidateMethod(MethodNode declaredMethod) { !GROOVY_OBJECT_CLASS_NODE.hasMethod(declaredMethod.getName(), declaredMethod.getParameters()); } - private static Parameter[] copyParameters(Parameter[] parameterTypes) { + private static Parameter[] copyParameters(Parameter... parameterTypes) { Parameter[] newParameterTypes = new Parameter[parameterTypes.length]; for (int i = 0; i < parameterTypes.length; i++) { Parameter parameterType = parameterTypes[i]; @@ -236,6 +237,7 @@ public int priority() { } private static class NoopDelegateAsyncTransactionalMethodTransformer implements DelegateAsyncTransactionalMethodTransformer { + @Override public void transformTransactionalMethod(ClassNode classNode, ClassNode delegateClassNode, MethodNode methodNode, ListExpression promiseDecoratorLookupArguments) { // noop } diff --git a/grails-async/gpars/src/main/groovy/org/grails/async/factory/gpars/LoggingPoolFactory.groovy b/grails-async/gpars/src/main/groovy/org/grails/async/factory/gpars/LoggingPoolFactory.groovy index 3709b412697..57d269346f5 100644 --- a/grails-async/gpars/src/main/groovy/org/grails/async/factory/gpars/LoggingPoolFactory.groovy +++ b/grails-async/gpars/src/main/groovy/org/grails/async/factory/gpars/LoggingPoolFactory.groovy @@ -48,7 +48,7 @@ class LoggingPoolFactory implements PoolFactory { private static final long KEEP_ALIVE_TIME = 10L public static final Logger LOG = LoggerFactory.getLogger(LoggingPoolFactory) - public static Method createThreadNameMethod + public static final Method createThreadNameMethod static { createThreadNameMethod = DefaultPool.getDeclaredMethod('createThreadName') diff --git a/grails-async/plugin/src/main/groovy/grails/async/services/PersistenceContextPromiseDecorator.groovy b/grails-async/plugin/src/main/groovy/grails/async/services/PersistenceContextPromiseDecorator.groovy index bc5b924a850..4c75bb615a9 100644 --- a/grails-async/plugin/src/main/groovy/grails/async/services/PersistenceContextPromiseDecorator.groovy +++ b/grails-async/plugin/src/main/groovy/grails/async/services/PersistenceContextPromiseDecorator.groovy @@ -20,8 +20,8 @@ package grails.async.services import groovy.transform.CompileStatic -import grails.persistence.support.PersistenceContextInterceptorExecutor import grails.async.decorator.PromiseDecorator +import grails.persistence.support.PersistenceContextInterceptorExecutor /** * A {@link PromiseDecorator} that wraps a promise execution in a persistence context (example Hibernate session) diff --git a/grails-async/plugin/src/main/groovy/grails/async/web/AsyncGrailsWebRequest.groovy b/grails-async/plugin/src/main/groovy/grails/async/web/AsyncGrailsWebRequest.groovy index 66387a99818..ebbc8581ddb 100644 --- a/grails-async/plugin/src/main/groovy/grails/async/web/AsyncGrailsWebRequest.groovy +++ b/grails-async/plugin/src/main/groovy/grails/async/web/AsyncGrailsWebRequest.groovy @@ -19,16 +19,11 @@ package grails.async.web -import groovy.transform.CompileStatic -import org.grails.web.util.GrailsApplicationAttributes -import org.grails.web.servlet.mvc.GrailsWebRequest -import org.springframework.context.ApplicationContext -import org.springframework.util.Assert -import org.springframework.web.context.request.async.AsyncWebRequest - import java.util.concurrent.atomic.AtomicBoolean import java.util.function.Consumer +import groovy.transform.CompileStatic + import jakarta.servlet.AsyncContext import jakarta.servlet.AsyncEvent import jakarta.servlet.AsyncListener @@ -36,6 +31,13 @@ import jakarta.servlet.ServletContext import jakarta.servlet.http.HttpServletRequest import jakarta.servlet.http.HttpServletResponse +import org.springframework.context.ApplicationContext +import org.springframework.util.Assert +import org.springframework.web.context.request.async.AsyncWebRequest + +import org.grails.web.servlet.mvc.GrailsWebRequest +import org.grails.web.util.GrailsApplicationAttributes + /** * Implementation of Spring 4.0 {@link AsyncWebRequest} interface for Grails * diff --git a/grails-async/plugin/src/main/groovy/org/grails/async/transform/internal/DefaultDelegateAsyncTransactionalMethodTransformer.groovy b/grails-async/plugin/src/main/groovy/org/grails/async/transform/internal/DefaultDelegateAsyncTransactionalMethodTransformer.groovy index bc2df334006..56bc722de53 100644 --- a/grails-async/plugin/src/main/groovy/org/grails/async/transform/internal/DefaultDelegateAsyncTransactionalMethodTransformer.groovy +++ b/grails-async/plugin/src/main/groovy/org/grails/async/transform/internal/DefaultDelegateAsyncTransactionalMethodTransformer.groovy @@ -44,9 +44,9 @@ import org.codehaus.groovy.syntax.Types import org.springframework.transaction.PlatformTransactionManager import org.springframework.transaction.annotation.Transactional +import grails.transaction.TransactionManagerAware import org.grails.compiler.injection.GrailsASTUtils import org.grails.compiler.web.async.TransactionalAsyncTransformUtils -import grails.transaction.TransactionManagerAware /** * Modifies the @DelegateAsync transform to take into account transactional services. New instance should be created per class transform, as state is held. @@ -61,7 +61,9 @@ class DefaultDelegateAsyncTransactionalMethodTransformer implements DelegateAsyn private static final ClassNode TRANSACTIONAL_CLASS_NODE = new ClassNode(Transactional) private static final ClassNode INTERFACE_TRANSACTION_MANAGER = new ClassNode(PlatformTransactionManager).getPlainNodeReference() private static final ClassNode INTERFACE_TRANSACTION_MANAGER_AWARE = new ClassNode(TransactionManagerAware).getPlainNodeReference() - private static final Parameter[] SET_TRANSACTION_MANAGER_METHOD_PARAMETERS = [new Parameter(INTERFACE_TRANSACTION_MANAGER, 'transactionManager')] as Parameter[] + private static final Parameter[] SET_TRANSACTION_MANAGER_METHOD_PARAMETERS = [ + new Parameter(INTERFACE_TRANSACTION_MANAGER, 'transactionManager') + ] as Parameter[] private static final String FIELD_NAME_TRANSACTION_MANAGER = '$transactionManager' private static final String FIELD_NAME_PROMISE_DECORATORS = '$promiseDecorators' private static final ClassNode CLASS_NODE_MAP = new ClassNode(Map).getPlainNodeReference() diff --git a/grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/AsyncWebRequestPromiseDecorator.groovy b/grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/AsyncWebRequestPromiseDecorator.groovy index 5303f5fd649..28bc9f1b050 100644 --- a/grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/AsyncWebRequestPromiseDecorator.groovy +++ b/grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/AsyncWebRequestPromiseDecorator.groovy @@ -32,10 +32,10 @@ import org.springframework.web.context.request.RequestContextHolder import org.springframework.web.context.request.async.WebAsyncManager import org.springframework.web.context.request.async.WebAsyncUtils +import grails.async.decorator.PromiseDecorator import grails.async.web.AsyncGrailsWebRequest import org.grails.web.servlet.mvc.GrailsWebRequest import org.grails.web.util.WebUtils -import grails.async.decorator.PromiseDecorator /** * A promise decorated lookup strategy that binds a WebRequest to the promise thread diff --git a/grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/spring/PromiseFactoryBean.groovy b/grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/spring/PromiseFactoryBean.groovy index 7adc2eb6684..8bebcdfe662 100644 --- a/grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/spring/PromiseFactoryBean.groovy +++ b/grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/spring/PromiseFactoryBean.groovy @@ -52,5 +52,4 @@ class PromiseFactoryBean extends PromiseFactoryBuilder implements FactoryBean Date: Wed, 27 May 2026 21:09:46 -0400 Subject: [PATCH 2/2] Address Copilot review feedback Fixes four Copilot review comments on the async defensive cleanup: 1. AbstractPromiseFactory.createPromise(List, List) on null input Previously returned `new PromiseList()`. An empty PromiseList delegates `onComplete`/`onError` to the active PromiseFactory's `onComplete(List, Closure)`, which (in CachedThreadPoolPromiseFactory) uses `while (promises.every { !promise.isDone() })`. Because Groovy's `every` returns `true` for an empty collection (vacuous truth), this loop would never terminate and any caller registering a completion handler on the empty promise would busy-wait forever. Now returns `new BoundPromise>(Collections. emptyList())`, which is an already-resolved promise that invokes onComplete callbacks synchronously with the empty list value. 2. FutureTaskPromise.set / setException null-checks removed `successCallbacks` and `failureCallbacks` are declared `final` and initialized at the field declaration with `new ConcurrentLinkedQueue<>()`. They cannot be null, so the defensive null-checks added in the previous commit were dead code and misleading about the field nullability. 3. Add regression test for the null-closures path New spec method in SynchronousPromiseFactorySpec verifies that `createPromise((List)null, decorators)` returns an already-done promise resolving to an empty list and that an `onComplete` callback registered on it is invoked synchronously with the empty list rather than busy-waiting. Assisted-by: claude-code:claude-4.7-opus --- .../factory/AbstractPromiseFactory.groovy | 2 +- .../factory/future/FutureTaskPromise.groovy | 16 +++++------- .../SynchronousPromiseFactorySpec.groovy | 25 ++++++++++++++++++- 3 files changed, 31 insertions(+), 12 deletions(-) diff --git a/grails-async/core/src/main/groovy/grails/async/factory/AbstractPromiseFactory.groovy b/grails-async/core/src/main/groovy/grails/async/factory/AbstractPromiseFactory.groovy index 52105467a94..3bc51a98cc7 100644 --- a/grails-async/core/src/main/groovy/grails/async/factory/AbstractPromiseFactory.groovy +++ b/grails-async/core/src/main/groovy/grails/async/factory/AbstractPromiseFactory.groovy @@ -85,7 +85,7 @@ abstract class AbstractPromiseFactory implements PromiseFactory { */ Promise> createPromise(List> closures, List decorators) { if (closures == null) { - return new PromiseList() + return new BoundPromise>(Collections. emptyList()) } List> decoratedClosures = new ArrayList>(closures.size()) for (Closure closure : closures) { diff --git a/grails-async/core/src/main/groovy/org/grails/async/factory/future/FutureTaskPromise.groovy b/grails-async/core/src/main/groovy/org/grails/async/factory/future/FutureTaskPromise.groovy index 2dcfb58e43b..66f72568710 100644 --- a/grails-async/core/src/main/groovy/org/grails/async/factory/future/FutureTaskPromise.groovy +++ b/grails-async/core/src/main/groovy/org/grails/async/factory/future/FutureTaskPromise.groovy @@ -84,11 +84,9 @@ class FutureTaskPromise extends FutureTask implements Promise { @Override protected void set(T t) { super.set(t) - if (successCallbacks != null) { - synchronized (successCallbacks) { - for (FutureTaskChildPromise callback : successCallbacks) { - callback.accept(t) - } + synchronized (successCallbacks) { + for (FutureTaskChildPromise callback : successCallbacks) { + callback.accept(t) } } } @@ -96,11 +94,9 @@ class FutureTaskPromise extends FutureTask implements Promise { @Override protected void setException(Throwable t) { super.setException(t) - if (failureCallbacks != null) { - synchronized (failureCallbacks) { - for (FutureTaskChildPromise callback : failureCallbacks) { - callback.accept(t) - } + synchronized (failureCallbacks) { + for (FutureTaskChildPromise callback : failureCallbacks) { + callback.accept(t) } } } diff --git a/grails-async/core/src/test/groovy/grails/async/SynchronousPromiseFactorySpec.groovy b/grails-async/core/src/test/groovy/grails/async/SynchronousPromiseFactorySpec.groovy index e4c96228a94..08293af317b 100644 --- a/grails-async/core/src/test/groovy/grails/async/SynchronousPromiseFactorySpec.groovy +++ b/grails-async/core/src/test/groovy/grails/async/SynchronousPromiseFactorySpec.groovy @@ -164,4 +164,27 @@ class SynchronousPromiseFactorySpec extends Specification { then: 'the closure is executed twice' 2 * callable.call() } -} \ No newline at end of file + + void 'createPromise with a null closures list returns a completed empty-list promise'() { + + given: 'a factory and a decorator list' + def factory = Promises.promiseFactory + def decorators = [{ Closure c -> c } as PromiseDecorator] + + when: 'createPromise is called with a null closures list' + def promise = factory.createPromise((List>) null, decorators) + + then: 'the returned promise is already done and resolves to an empty list' + promise.isDone() + promise.get() == [] + + when: 'onComplete is registered on the resulting promise' + def callbackResult = null + def callbackInvoked = false + promise.onComplete { value -> callbackInvoked = true; callbackResult = value } + + then: 'the callback is invoked synchronously with an empty list rather than busy-waiting' + callbackInvoked + callbackResult == [] + } +}