Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

retry / repeat (was: retryWithDelay) #15

Closed
brunogb opened this issue Apr 13, 2016 · 11 comments
Closed

retry / repeat (was: retryWithDelay) #15

brunogb opened this issue Apr 13, 2016 · 11 comments

Comments

@brunogb
Copy link

brunogb commented Apr 13, 2016

Name and description

retryWithDelay(numberOfAttempts: Int, delayIncrease: Double)

Motivation for inclusion

Allows an operation to wait some time before trying again in case of error. Also allow for increment the waiting after each failed attempt

Example of use

We could use this operator like a "Retry to connect in" that we seen in many communicators app.
Sometimes it may be better to wait some time before trying an operation again.

Suggested implementation

This was a suggested implementation made by @mjohnson (on rxSwift slack). I plan to improve in this solution allowing for better parametrisation

.retryWhen { (attempts) in
        return Observable.zip(attempts, Observable.range(start: 1, count: 4)) { n, i in
            return (n, i)
            }.flatMap { (n, i) in

                return i >= 4 ? Observable.error(n) : Observable<Int>.timer(RxTimeInterval(i), scheduler: MainScheduler.instance)
        }
@fpillet
Copy link
Member

fpillet commented Apr 13, 2016

I'd like to explore the possibility of having a dynamic retry mechanism. This would allow for exponential backoff scenarios, provided we do something like this:

RetryHandler would take the number of attempts so far, the delay that was applied for each attempt
and return a value >= 0 to retry, < 0 to stop retrying. This is not an ideal situation,

enum RetryChoice {
  case Immediate
  case Delayed(Double)
  case Cancelled
}
typealias RetryHandler = (attempts: Int, delays: [RetryChoice]) -> RetryChoice

extension ObservableType {
  func retry(handler: RetryHandler) -> Observable<E> {
    // ...
  }
}

Opinions?

@fpillet
Copy link
Member

fpillet commented Apr 15, 2016

Ok more thoughts about this excellent idea for an operator. I think we can implement several possible variations to cover the full spectrum:

Retry with a fixed delay at each iteration:
retry(maxAttemptCount : Int, delay : Double)

Retry with a fixed delay increase at each iteration, optionally providing the initial delay:
retry(maxAttemptCount : Int, initialDelay : Double = 0, delayIncrease : Double)

Retry with a growing delay as a percentage of previous delay:
retry(maxAttemptCount : Int, initialDelay : Double, delayPercentIncrease : Double)

I am tempted to make maxAttemptCount an @autoclosure () -> Int instead of a simple Int. This would let developer pass a function that can conditionally cancel the delayed retry. But I fear this may only confuse most readers. Moreover, for complex cases one can use retryWhen as the implementation for this operator will do.

@fpillet
Copy link
Member

fpillet commented Apr 15, 2016

I think we should also add a scheduler parameter (with a default going to the MainScheduler.instance).

Therefore I suggest we do:

Retry with a fixed delay at each iteration:
retry(maxAttemptCount : Int, delay : Double, scheduler : SchedulerType = MainScheduler.instance)

Retry with a fixed delay increase at each iteration, optionally providing the initial delay:
retry(maxAttemptCount : Int, initialDelay : Double = 0, delayIncrease : Double, scheduler : SchedulerType = MainScheduler.instance)

Retry with a growing delay as a percentage of previous delay:
retry(maxAttemptCount : Int, initialDelay : Double, delayPercentIncrease : Double, scheduler : SchedulerType = MainScheduler.instance)

@brunogb what do you think about this?

@brunogb
Copy link
Author

brunogb commented Apr 15, 2016

Hey @fpillet nice suggestions :)

I think we could keep the enum approach and improve in the number of cases to cover full spectrum?
Also totally agree to pass an scheduler also.
Also i think we could provide a case where the delay could be calculated based on the retry counter. Maybe after the third try, i would like to wait a minute to try again.

enum RetryChoice {
  case Immediate
  case Delayed(Double)
  case ExponentialDelayed(Double, Double)
  case PercentageDelayed(Double, Double)
  case CustomDelayTimer((Int-> Double))
  case Cancelled
}

extension ObservableType {
  func retry(maxAttempts: Int, choice: RetryChoice, scheduler : SchedulerType = MainScheduler.instance) -> Observable<E> {
    // ...
  }
}

What do you think?

@fpillet
Copy link
Member

fpillet commented Apr 15, 2016

The last one .Cancelled doesn't make sense until it is returned by a closure during a retry. For enum cases with multiple parameters I think we should use named parameters otherwise reading the code may not be clear (maybe I'm splitting hair but I think making the intent obvious a an important thing)

Also, since we are passing an enum, why not make maxAttempts part of the enum case ?

@brunogb
Copy link
Author

brunogb commented Apr 15, 2016

Ah yes, I thought for the real code to have named parameters :)

Should be something like this then:

enum RetryChoice {
  case Immediate (attempts: Int)
  case Delayed (attempts: Int, time: Double)
  case ExponentialDelayed (attempts: Int, initial: Double, multiplier: Double)
  case PercentageDelayed (attempts: Int, initial: Double, percent: Double)
  case CustomDelayTimer (attempts: Int, delayCalculator:(Int-> Double))
}

extension ObservableType {
  func retry(choice: RetryChoice, scheduler : SchedulerType = MainScheduler.instance) -> Observable<E> {
    // ...
  }
}

Also, maybe ExponentialDelayed and PercentageDelayed could be just one. If you pass an double like 1.7 you will be incrementing 70% on each retry. So we could end with 4 cases:

enum RetryChoice {
  case Immediate (attempts: Int)
  case Delayed (attempts: Int, time: Double)
  case ExponentialDelayed (attempts: Int, initial: Double, multiplier: Double)
  case CustomDelayTimer (attempts: Int, delayCalculator:(Int-> Double))
}

@brunogb
Copy link
Author

brunogb commented Apr 15, 2016

Also we could rename Choice to Behavior:

enum RetryBehavior {
  case Immediate (attempts: Int)
  case Delayed (attempts: Int, time: Double)
  case ExponentialDelayed (attempts: Int, initial: Double, multiplier: Double)
  case CustomTimerDelayed (attempts: Int, delayCalculator:(Int-> Double))
}

extension ObservableType {
  func retryWithBehavior(behavior: RetryBehavior, scheduler : SchedulerType = MainScheduler.instance) -> Observable<E> {
    // ...
  }
}

@fpillet
Copy link
Member

fpillet commented Apr 15, 2016

I like this. I'd also like to add something similar for regular observable completion (repeatWithBehavior) once we're settled with this one.

@fpillet
Copy link
Member

fpillet commented Apr 15, 2016

Actually while we are at it, we could go as far as:

enum RepeatBehavior {
  case Immediate (attempts: Int = .max)
  case Delayed (attempts: Int, time: Double)
  case ExponentialDelayed (attempts: Int, initial: Double, multiplier: Double)
  case CustomTimerDelayed (attempts: Int, delayCalculator:(Int-> Double))
}

typealias RepeatPredicate = (ErrorType) -> Bool

extension ObservableType {
  // retry with behavior on .Error. Complete on .Completed
  func retry(behavior: RepeatBehavior, shouldRetry : RepeatPredicate? = nil, scheduler : SchedulerType = MainScheduler.instance) -> Observable<E> { }

  // repeat with behavior on .Completed. Don't repeat on error
  func repeat(behavior: RepeatBehavior, shouldRepeat : RepeatPredicate? = nil, scheduler : SchedulerType = MainScheduler.instance) -> Observable<E> { }
}

That is, have a condition in addition to a behavior. Is that too much? I don't think so, as long as there's a default parameter for unconditional behavior.

@fpillet fpillet changed the title retryWithDelay retry / repeat (was: retryWithDelay) May 2, 2016
@thanegill thanegill mentioned this issue May 17, 2016
@fpillet
Copy link
Member

fpillet commented Jul 26, 2016

So thanks for @reloni's contribution, we now have a retry implementation that performs the behavior on errors. Now we need a version of repeat (that performs the behavior on complete) as well. Any takers? :)

@icanzilb
Copy link
Member

icanzilb commented Aug 5, 2016

I love the retry operator, I think it'd work perfectly ported to pure Swift 👍

@icanzilb icanzilb self-assigned this Aug 5, 2016
@fpillet fpillet closed this as completed Oct 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants