Retry if the future itself fails

[GitsMcGee]

I often get bitten by the fact that the retry logic will not retry if an exception gets thrown within the body of the future. The workaround I have been using in the past is to have the future wrap a scala.util.Try. Doing so works, but is unnecessary since Try is already incorporated into scala.concurrent.Future (see Future.onComplete, for instance). So having to deal with a Future[Try[T]] is a little redundant and unnecessarily cumbersome.

As a solution, I added a simple recoverWith block that will perform a retry (and decrement the max retries) if the future fails. I also added a unit test

[softprops]

I'm a little against this because it suites a specific use case and prevents a potentially wanted exception from bubbling up. My general option is that yea runtime exceptions suck but at the same time if you know your future can throw an exception you should by wrapping it with a Try or some other machinery.

The role of retry in this case is to make Success something an application defines. Retry exposes a handful of common ways of defining success which includes a failed try. The general idea is that a computation should return some value and that value should encode a failure then retry will detect that and reattempt the operation.

[GitsMcGee]

Hi @softprops,

Yeah, I hear you. I don't necessarily disagree with anything you said, other than that I don't believe that this change will necessarily prevent a wanted exception from bubbling up. It just bubbles up the exception on the Nth attempt instead of the first. That should be totally safe unless you are doing something crazy side-effecty. The way I always saw it, retry wants you to define success, and anything that doesn't meet the criteria of Success (including exceptions being thrown) is therefore a failure.

Also, I wish I understood the design intention of Future a little better. It seems to me that it integrates Try already in a way (Future.onComplete for example), so forcing a wrap with a Try in all cases seemed redundant.

Anyway, just my 2 cents :)

Joe

[softprops]

I'll have a closer look this weekend. I may have missed something.

[softprops]

So I'm just now finding myself running into this issue and I think what you added makes good sense. I already have another branch that I've been working on with more cohesive abstractions which this will probably conflict with. I'll try to merge this in and fix conflicts on my end. Thanks again for putting effort into this.

[GitsMcGee]

Great to hear! This I run into this issue quite a bit, so this will help me a lot. Looking forward to the next release.