jamesfredley commented on code in PR #16619:
URL: https://github.com/apache/grails-core/pull/16619#discussion_r4339239111
##########
grails-async/rxjava3/src/main/groovy/org/grails/async/factory/rxjava3/RxPromiseFactory.groovy:
##########
@@ -76,24 +76,27 @@ class RxPromiseFactory extends AbstractPromiseFactory {
@Override
<T> Promise<T> onComplete(List<Promise<T>> promises, Closure<T> callable) {
- new RxPromise<T>(this, Observable.concat(
- promises.collect { Promise p ->
- if (p instanceof BoundPromise) {
- return Observable.just(((BoundPromise) p).value) as
Observable<T>
- }
- else {
- return ((RxPromise) p).toObservable() as Observable<T>
- }
- }
- ).toList())
- .onComplete(callable)
+ new RxPromise<T>(this, values(promises)).onComplete(callable)
}
@Override
<T> Promise<List<T>> onError(List<Promise<T>> promises, Closure<?>
callable) {
- new RxPromise(this, Observable.concat(
- promises.collect { ((RxPromise<T>) it).toObservable() }
- ).toList())
- .onError(callable) as Promise<List<T>>
+ new RxPromise(this, values(promises)).onError(callable) as
Promise<List<T>>
+ }
+
+ /**
+ * Concatenates the values of the given promises into one list, in order,
with a {@code null} value restored
+ * from the sentinel it travels through the stream as.
+ */
+ private static <T> Single<List<T>> values(List<Promise<T>> promises) {
+ List<Observable<T>> sources = promises.collect { Promise<T> p ->
+ if (p instanceof BoundPromise) {
+ return Observable.just(RxPromise.wrap(((BoundPromise)
p).value)) as Observable<T>
Review Comment:
`BoundPromise.then()` stores a thrown exception in `value`
(`BoundPromise.groovy` around lines 88-94). `get()` treats that value as
failure. This branch emits it through `Observable.just`, so a `Throwable` is a
successful list element.
```groovy
def failure = new IllegalStateException('boom')
def failed = Promises.createBoundPromise(1).then { throw failure }
Throwable seen = null
def result = Promises.onError([failed]) { seen = it }.get()
// result == [failure], seen == null
```
List `onComplete` has the same hole. `waitAll` does not, because it calls
`p.get()`.
Emit `Observable.error((Throwable) value)` when the stored value is a
`Throwable`, and emit the wrapped value otherwise. Please add a public-API
regression: a bound promise whose `then` throws must invoke list `onError` with
the original exception, fail aggregate `get()`, and must not invoke list
`onComplete`. Keep the null-value and single-execution tests.
--
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]
