Rxswift: RetryWhen operator behaviour with completed trigger

[bontoJR]

I was playing heavily with retryWhen operator and noticed a really weird behaviour, if the trigger completes, also the original observable completes, which, to be frankly, it's an unexpected behaviour.

If the outer observable errors out and the inner trigger completes, the operator should pass on the last error which might be then caught.

Current behaviour

O: ----1----2----X----1----2----X
T: --------------T--------------C
R: ----1----2---------1----2----C

When I think it would make more sense to have:

O: ----1----2----X----1----2----X
T: --------------T--------------C
R: ----1----2---------1----2----X

This consideration comes on thinking about retry(X) and retry which would forward the error if the number of times of retry(maxAttempts) has been completed unsuccessfully. retryWhen is kind-of a super set of both retry operators, with a finite trigger sequence for the max attempt and an infinite one for unlimited retry, so it's a little bit illogical to terminate a sequence which previously had an error.

Open for discussions anyway. :)

[icanzilb]

as always - any other Rx implementations that already feature retryWhen(), that we can have a look into?

[bontoJR]

retryWhen from rxJS which operates in the way I am suggesting.

[bontoJR]

I have to correct myself, the implementation of rxJS changed after my first check when I was working on this operator, so since November the behaviour has been changed from forwarding error to complete.

In RxJava the implementation is different, so for retryWhen there's no onNext on the trigger, but only onError: source. Well this second option looks way more interesting to be honest:

The Observable input only triggers on terminal events

From a logic point of view this solution looks even better than mine. There's no onNext on the trigger, but the onError to trigger the re-subscription and onComplete to forward the error down in the sequence.

It also adds some good points to add repeatWhen, related to #396.

To recap the suggested way is:

O: ----1----2----X----1----2----X
T: --------------E--------------C
R: ----1----2---------1----2----X

[fpillet]

I gave a second though to that, following our discussion on Slack. To me it seems that the (undocumented) way it behaves now is the most flexible. The trigger sequence is the ultimate decision maker on what happens when the source sequence errors out.

Now since you control the trigger sequence, there is no good reason not to pass errors along to the outer observable when you want to error out. The ability to complete while silencing the last errors actually seems like a very nice feature! It spares you from adding a catchError to the sequence if all you want is retries up to a point and then just complete if nothing worked.

At the very least, this behaviour should be properly documented in RxSwift (maybe worth a post, Marin?) as it seems it could be a handy feature in some scenarios.

Unless I completely misunderstood your issue, I like the current behaviour. This lets you do things like:

Silencing the last error:
O: ----1----2----X----1----2----X
T: --------------E--------------C
R: ----1----2---------1----2----C

and

Propagating the last error:
O: ----1----2----X----1----2----X
T: --------------E--------------X
R: ----1----2---------1----2----X

More choice is good, IMHO

[sergdort]

Hi, guys I would agree with @bontoJR
It seems like silencing error is not responsibility of this operator.
Please correct me if I'm wrong but looks like its behaviour is documented:

If the source observable errors and the notifier completes, it will complete the source sequence.

[sergdort]

Lets say I want to retry with delay, does it mean that current behaviour will silence last error, if so I would not want this behaviour

mySource.tetryWhem { _ in
      return Observable<Int>.interval(2, scheduler).take(3)
}

[bontoJR]

@fpillet I would like to focus on this:

To me it seems that the (undocumented) way it behaves now is the most flexible.

This operator is capable of:

• Recovering
• Forwarding
• Completing

In my opinion there's too much in there, the trigger can't have so much power over the source observable and the power of Rx is also that operators have the right amount of flexibility and responsibility. The operator has the name retryWhen and this implies that completion is not a potential outcome. I would prefer a usage of retryWhen that can then be paired with catch because that operator is meant to catch an error and recover the sequence.

That's my main concern, this operator has too much power given to the trigger observable.

I agree on @sergdort example, that code should forward the error after the trigger completes, not to silence the error, at least this was my expected outcome also considering that retryWhen should be able to reproduce retry() and retry(attempts) in a straightforward way. :)

[kzaher]

Hi guys,

interesting discussion :) I totally understand what @bontoJR is saying, that behavior was also weird to me when we implemented it.

I think we just copied behavior of some rx implementation. Probably rxjs.

I don't think I've ever relied in my code on completing retryWhen sequence. I've always either relied on either returning element or erroring out sequence if I wanted it to stop so either way is fine by me.

Summary:

• I'm reluctant to call this a bug because we do have unit test written for this and they do document this behavior.
• It would be nice to find out why rxjs has that completed behavior, maybe like @fpillet says, they've done it for extra flexibility, or maybe there is some other compelling reason.
• I have no idea how should this behave if the only two other rx libraries that have this operator implemented behave differently. Since I'm fine going both ways, the most rational thing will probably be => whichever other rx library will be most popular when we'll be releasing RxSwift 4.0, we'll adapt their behavior.
  ¯\_(ツ)_/¯

[bontoJR]

Hello @kzaher

I'm reluctant to call this a bug because we do have unit test written for this and they do document this behavior.

For the record, no one here mentioned it's a bug, in fact this is a really amazing open discussion about an operator. :D

It would be nice to find out why rxjs has that completed behavior, maybe like @fpillet says, they've done it for extra flexibility, or maybe there is some other compelling reason.

Yes, that would make sense, in fact the original implementation was as I was actually recommending and it has been changed in rxJS 5.0 (November 2016 ?).

=> whichever other rx library will be most popular when we'll be releasing RxSwift 4.0, we'll adapt their behavior.

AHAHAHA I loved this! 👍

To me it would make more sense to follo RxJava here, but I might be wrong... or we can post a poll and vote? Anyway, any information about the repeatWhen, do you think it might be useful?

[kzaher]

Hi,

It seems to me that currently RxJava is the most popular implementation. As far as I can tell when error transformation block completes sequence doesn't terminate with latest error.

Observable.just(0xf).concatWith(Observable.<Integer>error(new Throwable())).retryWhen(new Function<Observable<Throwable>, ObservableSource<?>>() {
            @Override
            public ObservableSource<?> apply(@NonNull Observable<Throwable> throwableObservable) throws Exception {
                return Observable.zip(throwableObservable, Observable.range(0, 3), new BiFunction<Throwable, Integer, Integer>() {
                    @Override
                    public Integer apply(@NonNull Throwable throwable, @NonNull Integer integer) throws Exception {
                        return integer;
                    }
                }).switchMap(new Function<Integer, ObservableSource<Long>>() {
                    @Override
                    public ObservableSource<Long> apply(@NonNull Integer o) throws Exception {
                        return Observable.timer(o.intValue() * 50, TimeUnit.MILLISECONDS, Schedulers.io());
                    }
                });
            }
        }).subscribe(new Consumer<Integer>() {
            @Override
            public void accept(@NonNull Integer integer) throws Exception {
                System.out.println(integer);
            }
        }, new Consumer<Throwable>() {
            @Override
            public void accept(@NonNull Throwable throwable) throws Exception {
                System.out.println(throwable);
            }
        }, new Action() {
            @Override
            public void run() throws Exception {
                System.out.println("Completed");
            }
        });

I kind of understand why that might be the case and since we are already consistent with RxJava it seems to me it might be better to leave the current behavior.