cats-effect icon indicating copy to clipboard operation
cats-effect copied to clipboard

Retry functionality

Open SystemFw opened this issue 3 years ago • 4 comments

Fixes #1459

I had originally started with a direct port of cats-retry, but since then I've changed the design significantly enough to warrant a proper review of the api before I port tests and docs. Everything is in Retry.scala, here are some of the relevant decisions:

  • The time scheduling part of the problem is mostly the same as cats-retry, except with fewer types.
  • The retry api itself has changed significantly, cats-retry has several combinators, each of which takes several callbacks, whereas I've folded that into the Retry abstraction itself. I've found the deluge a similar combinators in cats-retry to be a bit hard to navigate, plus you can't have default implementations for the callbacks because they are of shape X => F[Y].
  • I've removed the pretty printing of retries, we don't do that anywhere else in cats-effect.
  • I've removed the ability to retry with a predicate on the result of the action, with the rationale that you can use failures and ensure instead.
  • The errors are typed at Throwable (could be given the Gen* treatment though I'm not especially keen).

TODO:

  • [x] Syntax. Should be minimal given the reduction to a single retry combinator. ~~I'm afraid usage with IO would require a syntax import since this lives in std.~~
  • [ ] Pass at the scaladoc.
  • [ ] Porting tests.

SystemFw avatar Sep 04 '22 22:09 SystemFw

I'm afraid usage with IO would require a syntax import since this lives in std.

The core module depends on std, so syntax imports shouldn't be necessary, we can add methods directly to IO. Unless I misunderstood what you are getting at :)

armanbilge avatar Sep 05 '22 00:09 armanbilge

By the way, I'm explicitly seeking feedback on the interaction between selectError and and/or with timed retry policies. I think in common usage it's fine, but I also think it's possible to build confusing behaviour in certain cases. However, the alternative is writing lots of overloads for retry combinators with callbacks, and forcing the user to bundle timing logic and error selection logic manually

SystemFw avatar Sep 05 '22 21:09 SystemFw

I also think it's possible to build confusing behaviour in certain cases

would you mind showing an example of this?

armanbilge avatar Sep 05 '22 21:09 armanbilge

would you mind showing an example of this?

For example capDelay used to be implemented as _.or(constantDelay(cap)). But if you use that with a policy that works on a specific error, e.g.

exponentialBackoff(1.ms)
  .selectError { 
       case NullPointerException => true
       case _ => false
    }.capDelay(500.ms) // with the old implementation of capDelay

the resulting policy will retry all errors, not just NullPointerException. Another example would be anding policies that select non-overlapping errors, like constantDelay.selectError(..ExA..).and(constantDelay.selectError(...ExB...), which means you never retry (though this one should be quite obvious with the rename from join to and). All in all I think common usage is fine, since I'd imagine you first write the code that specifies the "scheduling" , and then selectError at the end, and also by following the simple rule of always having non-overlapping error selection for or, and overlapping error selection for and, but I think it's worth getting other people to think about it.

SystemFw avatar Sep 06 '22 09:09 SystemFw