jamesfredley commented on code in PR #15682:
URL: https://github.com/apache/grails-core/pull/15682#discussion_r3314752870
##########
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:
Good catch - confirmed the busy-wait would trigger because Groovy's `every`
on an empty collection returns `true` (vacuous truth), so `while
(promises.every { !promise.isDone() })` becomes an infinite loop. Fixed in
14216f8505 by returning `new BoundPromise<List<T>>(Collections.<T>
emptyList())` so the empty-list case is an already-resolved promise that
invokes callbacks synchronously.
##########
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:
Done in 14216f8505 - added `createPromise with a null closures list returns
a completed empty-list promise` to `SynchronousPromiseFactorySpec`. Verifies
the returned promise is already done, resolves to `[]`, and that `onComplete`
is invoked synchronously rather than busy-waiting. Verified passing locally.
##########
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:
Correct - the field is `private final Collection<FutureTaskChildPromise>
successCallbacks = new ConcurrentLinkedQueue<>()` so it can never be null. The
null-check was dead code. Removed in 14216f8505.
##########
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:
Same - the field is `final` and initialized at declaration with `new
ConcurrentLinkedQueue<>()` so it can never be null. Removed in 14216f8505.
--
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]
