You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@grails.apache.org by ja...@apache.org on 2026/05/27 21:02:42 UTC
(grails-core) 01/01: Async project defensive coding cleanup
This is an automated email from the ASF dual-hosted git repository.
jamesfredley pushed a commit to branch chore/async-defensive-cleanup
in repository https://gitbox.apache.org/repos/asf/grails-core.git
commit 00f1a3751ebc2ffe744328956c9c108a7486d8c6
Author: James Daugherty <jd...@jdresources.net>
AuthorDate: Wed May 13 21:43:28 2026 -0400
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
---
.../grails/async/factory/AbstractPromiseFactory.groovy | 3 +++
.../grails/async/factory/future/FutureTaskPromise.groovy | 16 ++++++++++------
.../transform/internal/DelegateAsyncTransformation.java | 8 +++++---
.../grails/async/factory/gpars/LoggingPoolFactory.groovy | 2 +-
.../services/PersistenceContextPromiseDecorator.groovy | 2 +-
.../groovy/grails/async/web/AsyncGrailsWebRequest.groovy | 16 +++++++++-------
...ultDelegateAsyncTransactionalMethodTransformer.groovy | 6 ++++--
.../web/async/AsyncWebRequestPromiseDecorator.groovy | 2 +-
.../plugins/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 caf480a3d2..52105467a9 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)
*/
<T> Promise<List<T>> createPromise(List<Closure<T>> closures, List<PromiseDecorator> decorators) {
+ if (closures == null) {
+ return new PromiseList<T>()
+ }
List<Closure<T>> decoratedClosures = new ArrayList<Closure<T>>(closures.size())
for (Closure<T> 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 66f7256871..2dcfb58e43 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<T> extends FutureTask<T> implements Promise<T> {
@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<T> extends FutureTask<T> implements Promise<T> {
@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 1b286533e7..a776e0644d 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 @@ public class DelegateAsyncTransformation implements ASTTransformation, Transform
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 @@ public class DelegateAsyncTransformation implements ASTTransformation, Transform
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 @@ public class DelegateAsyncTransformation implements ASTTransformation, Transform
!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 class DelegateAsyncTransformation implements ASTTransformation, Transform
}
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 3709b41269..57d269346f 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 bc5b924a85..4c75bb615a 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 66387a9981..ebbc8581dd 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 bc2df33400..56bc722de5 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 5303f5fd64..28bc9f1b05 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 7adc2eb668..8bebcdfe66 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<Pr
boolean isSingleton() {
return true
}
-
}