Copilot commented on code in PR #15682:
URL: https://github.com/apache/grails-core/pull/15682#discussion_r3313871094
##########
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>()
+ }
Review Comment:
Returning an empty PromiseList when `closures` is null can hang callers that
use `onComplete`/`onError` with the default `CachedThreadPoolPromiseFactory`:
its `onComplete(List, Closure)` busy-wait loop never terminates for an empty
`promises` list. To keep the new null-safety but avoid this behavior, return an
already-resolved promise of an empty list (for example a
`BoundPromise<List<T>>` / `createBoundPromise(Collections.emptyList())`)
instead of an empty `PromiseList`.
##########
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>()
Review Comment:
This new null-handling path changes API behavior but isn't covered by
existing async promise specs. Please add a test that `createPromise((List)null,
decorators)` returns a completed promise of an empty list and that `onComplete`
is invoked (to guard against regressions like empty-list busy-waits).
##########
grails-async/core/src/main/groovy/org/grails/async/factory/future/FutureTaskPromise.groovy:
##########
@@ -84,19 +84,23 @@ 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)
+ }
}
}
}
@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)
+ }
Review Comment:
`failureCallbacks` is declared `final` and initialized at field declaration,
so it cannot be null. The new null-check is redundant here as well; consider
removing it (or making the field genuinely nullable if that's required).
##########
grails-async/core/src/main/groovy/org/grails/async/factory/future/FutureTaskPromise.groovy:
##########
@@ -84,19 +84,23 @@ 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)
+ }
Review Comment:
`successCallbacks` is declared `final` and initialized at field declaration,
so it cannot be null. The new null-check is therefore redundant and adds
noise/misleading semantics. Either remove the null-check, or if null is
actually possible, make the field nullable and document/initialize it
consistently.
--
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]
