jamesfredley commented on code in PR #16690:
URL: https://github.com/apache/grails-core/pull/16690#discussion_r4338702133
##########
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()` catches a callback failure and stores it as the value.
`BoundPromise.get()` throws that stored failure. This branch emits `.value`
through `Observable.just`, so a `Throwable` is a successful list element.
List `onError` now calls `values()`, so this is a new silent-success path:
```groovy
def failure = new IllegalStateException('bad')
def failed = Promises.createBoundPromise(null).then { throw failure }
Throwable observed = null
def aggregate = Promises.onError([failed]) { observed = it }
aggregate.get() // returns [failure]; observed stays null
```
Previously this input threw `ClassCastException` because `onError` cast
every promise to `RxPromise`. That was wrong, but it did not report success.
Extract the bound result through `get()` inside a deferred source, and wrap
a successful null before emission, for example `Observable.fromCallable {
RxPromise.wrap(p.get()) }`. Please add a regression that a failed bound promise
invokes list `onError`, skips list `onComplete`, and makes the aggregate
`get()` throw. Keep the existing bound-null success coverage.
--
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]
