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

Add IO#retry and its variations #212

Closed
vitorsvieira opened this issue May 10, 2018 · 12 comments
Closed

Add IO#retry and its variations #212

vitorsvieira opened this issue May 10, 2018 · 12 comments

Comments

@vitorsvieira
Copy link

vitorsvieira commented May 10, 2018

Hi,

With the same purpose of #209, where it aims to make IO.timeout available, so users avoid copying/pasting the examples from the docs, I suggest the addition of IO.retry and its variations to IO's companion object.

By variations of the retry example in the docs, I'd mention:

@cb372 project cats-retry using cats-effect IO, insipired by Haskell's Control.Retry

And ioeffect:

  • IO.forever — Repeats the action until the first failure.
  • IO.retry — Repeats the action until the first success.
  • IO.retryN(n) — Repeats the action until the first success, for up to the specified number of times (n).
  • IO.retryFor(d) — Repeats the action until the first success, for up to the specified amount of time (d).

Reference: https://github.com/scalaz/ioeffect#retry

@johnynek
Copy link

I recently added forever to cats:
typelevel/cats#2249

@alexandru
Copy link
Member

I don't want to add those right now to be honest.

It increases the API surface and that's not a virtue, because in cats-effect they'll be basically set in stone. We also have a lot of things on our plate right now.

timeout is cool because there's no better place for it to be declared. retry on the other hand might be better somewhere else, maybe in MonadError.

@johnynek
Copy link

+1 that if possible we should put the methods as low as possible (forever works on FlatMap for instance, which can be useful with State even).

@pmellati
Copy link

We can also have a version of retry that takes a sequence of delays.

@SystemFw
Copy link
Contributor

That's the problem with retry, there can be a million different versions.
In fs2 we have a very general one, and a kitchen-sink one :P
https://github.com/functional-streams-for-scala/fs2/blob/series/0.10/core/shared/src/main/scala/fs2/Scheduler.scala#L196
https://github.com/functional-streams-for-scala/fs2/blob/series/0.10/core/shared/src/main/scala/fs2/Scheduler.scala#L171

The fact that the delays are an effectful Stream means you can compute them in all sorts of ways.

@vaslabs
Copy link

vaslabs commented Aug 10, 2018

I think this will be useful mainly to avoid repeating ourselves (and in reality we also need to have tests in each project). I include my proposal below that is specific to how we deal with this where I am but I believe is a common one.

The API should allow the user to do something like

def callThatCanFail(): Unit = ???
IO(callThatCanFail())
    .retry(
         backoffStrategy = Backoff.Exponential, 
         initialDelay = 2 seconds,
         maxRetries = 5
)

Alternatively we can accept a function for the backoff so we avoid the extra classes

def callThatCanFail(): Unit = ???

IO(callThatCanFail())
    .retry(
         backoffStrategy = _ * 2, 
         initialDelay = 2 seconds,
         maxRetries = 5
)

Another alternative as mentioned above is to accept a sequence or even a (finite?) stream of durations/delays.

I believe this way a lot of common cases are covered, for something more complicated just leave for the user to implement.

If you agree and nobody is already working on this I can start with a simple PR ?

@alexandru
Copy link
Member

@vaslabs repeating oneself is just one concern.

Cats-Effect is a library that sits lower in the stack of libraries one uses, being a standard library. Stability is a serious concern for Cats-Effect. Witness how Monix 3.0 has been continuously delayed because Cats-Effect 1.0 is not ready yet.

Therefore a small API surface is a virtue. The bigger the API, the higher the likelihood that 1.0 will get delayed again, or that compatibility will break again soon.

I personally do not want back-off strategies in Cats-Effect, first of all because I identified multiple back-off strategies in my own code and unfortunately there's no one size fits all. For example there are many cases in which repeating that call can be very expensive, so you might actually need to take the actual error into account.

Dealing with all of that, while dealing with the concerns of a standard library is really tough. Feature accretion for a standard library is a trap.

I find such an utility to be fine in Monix of course, but Monix has a slightly different profile. Such utilities can also live in other "contrib" libraries.

@vaslabs
Copy link

vaslabs commented Aug 10, 2018

Personally I don't mind if it's part of 1.0 or not, the work can be done after so it doesn't delay Monix.

If it's a no go forever, then it's still ok for me as I can look for other utilities as you suggest, but maybe we can make it clear and close the ticket. After-all the reason I'm asking is so I don't do throwaway work :)

Alternatively - after 1.0 - we can discuss if there is a good way of going forward and include it. Personally I agree with you about feature accretion and the complexity of figuring out different strategies, but I believe there is a balance, if the API is friendly and hides complicated things, it may hide traps, but it will be more attractive to the user. After-all a user that can recognise the benefits of different strategies and handling different errors depending on the behaviour of their side effects can go around the API to implement something better, but this is just my current opinion.

@SystemFw
Copy link
Contributor

SystemFw commented Aug 10, 2018

Although retry seems simple, there are a million strategies for doing it, and all of a sudden it's not just a retry function, you have a Backoff type and all, and I'm doubtful it belongs in cats-effect :)

I'm going to repeat what I said in the queue issue: I think at some point we'll need to start accepting that these things can live in other libraries that depend on cats-effect, rather than having everything here.

Note that for example we already have two retry combinators in fs2 (one with parameters, and a more flexible one based on streams). If you find yourself repeating the same a code a lot, have you considered creating a Retry library?

I think @ChristopherDavenport model is nice, he has a lot of small libraries that depend on cats-effect for all sort of small to medium tasks. The only problem with those is visibility, and I think we should start a discussion on that : how do we ensure that users of cats-effect are aware of other libraries in the ecosystem?

@alexandru
Copy link
Member

I think @ChristopherDavenport model is nice, he has a lot of small libraries that depend on cats-effect for all sort of small to medium tasks. The only problem with those is visibility, and I think we should start a discussion on that : how do we ensure that users of cats-effect are aware of other libraries in the ecosystem?

We can start promoting them on the website and the README, with a nice description of what they do.

The bigger problem right now though is the 1.0 release, which is actively hurting all of those potential libraries.

@vitorsvieira
Copy link
Author

Closing due to #301 #304

@alexandru
Copy link
Member

👍 we’ll publish the website soon too, after we release RC3; since we already have RC3 specific docs on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants