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

Dynamic delay (delay extracted from return value/exception) #63

Closed
runeksvendsen opened this issue Sep 26, 2019 · 10 comments

Comments

@runeksvendsen
Copy link
Contributor

@runeksvendsen runeksvendsen commented Sep 26, 2019

My app consumes an API which, for certain failure responses, will populate the Retry-After header of the response with the number of seconds to wait before performing the next request.

As far as I can see, the interface to this library doesn't support adjusting the delay time based on information in the return value of (or the exception thrown by) the action that's retried. It seems that all the functions that apply a RetryPolicy statically deduce the waiting time based on information inside RetryStatus.

Would it be within the scope of this library to have e.g. Control.Retry.retrying support dynamically adjusting the delay time based on information in the return value of the action to run (the b type variable)?

Seems to me like the way to support this feature would be to change the Bool return type, that's used to indicate whether a request should be retried, to something along the lines of:

data RetryAction
   = NoRetry                  -- ^ Don't retry
   | Retry                    -- ^ Retry with a delay calculated by the retry policy
   | RetryOverridingDelay Int -- ^ Retry, overriding the delay of the retry policy with the specified number of microseconds
@runeksvendsen runeksvendsen changed the title Dynamic delay (delay extracted from response/exception) Dynamic delay (delay extracted from return value/exception) Sep 26, 2019
@runeksvendsen

This comment has been minimized.

Copy link
Contributor Author

@runeksvendsen runeksvendsen commented Sep 30, 2019

I would like to implement the above suggestion. @MichaelXavier would you merge this?

@MichaelXavier

This comment has been minimized.

Copy link
Contributor

@MichaelXavier MichaelXavier commented Sep 30, 2019

Hmm. Right now we have a monoidal newtype over RetryStatus -> m (Maybe Int), which couldn't really know how long to wait unless perhaps m was a stateful monad which included Retry-After. So I think your intuition is right to try to do this in the "runner" function like retrying, perhaps calling it dynamicDelay or something. Couldn't you even rewrite retrying and possibly recovering in terms of this lower-level function? I think they would just never use the RetryOverridingDelay case. I think this is worth doing and I'd accept the PR.

@runeksvendsen

This comment has been minimized.

Copy link
Contributor Author

@runeksvendsen runeksvendsen commented Oct 1, 2019

Yes, the current versions of retrying and recovering could indeed be expressed in terms of the theoretical retryingDynamic and recoveringDynamic functions, respectively, since a Bool can easily be translated into a RetryAction:

toRetryAction :: Bool -> RetryAction
toRetryAction False = NoRetry
toRetryAction True = Retry

However, as far as I can see we would need to create both retryingDynamic and recoveringDynamic, ie. one for retrying without exceptions and one for retrying with exceptions.

One option is to make this a breaking change, so that both functions expect functions that return a RetryAction instead of a Bool (in the Handler m of recovering and the chk function passed to retrying). In addition we could export the above toRetryAction function, in order to make migration to this new version easier (library users would simply apply toRetryAction to the return values of their existing chk functions/Handlers). In this case I would also add an "upgrade guide" to the changelog, explaining this.

Another option would be to:

  1. Keep the existing retrying and recovering functions
  2. Add retryingDynamic and recoveringDynamic
  3. Redefine retrying and recovering in terms of retryingDynamic and recoveringDynamic, respectively
@runeksvendsen

This comment has been minimized.

Copy link
Contributor Author

@runeksvendsen runeksvendsen commented Oct 1, 2019

I would also like to note that custom retry delays in the Retry-After HTTP header isn't the only place where overriding the result of the RetryPolicy is relevant. If, for example, the error returned by an action is an error internal to the client (e.g. InternalException) there is no reason to do e.g. exponential backoff, and the client might as well retry immediately or after only a short delay.

@MichaelXavier

This comment has been minimized.

Copy link
Contributor

@MichaelXavier MichaelXavier commented Oct 1, 2019

I'd like to do your second option. I'm not entirely opposed to breaking changes but I don't think we should hassle users with having to fix all their use sites unless it really pays off to do so.

@runeksvendsen

This comment has been minimized.

Copy link
Contributor Author

@runeksvendsen runeksvendsen commented Oct 2, 2019

Great! I agree that it's probably best to minimize the amount of breaking changes.

I will open a PR when the code is done.

@runeksvendsen

This comment has been minimized.

Copy link
Contributor Author

@runeksvendsen runeksvendsen commented Oct 2, 2019

If you think of some good test cases please let me know.

@MichaelXavier

This comment has been minimized.

Copy link
Contributor

@MichaelXavier MichaelXavier commented Oct 2, 2019

Maybe have a canned sequence of delays, use the new feature with an action that pulls the delay out of the RetryStatus and asserts that the delays were the same and in the same order. Your monad could be just a pure Writer or State I think.

@runeksvendsen

This comment has been minimized.

Copy link
Contributor Author

@runeksvendsen runeksvendsen commented Oct 11, 2019

PR is up: #65

@MichaelXavier

This comment has been minimized.

Copy link
Contributor

@MichaelXavier MichaelXavier commented Oct 11, 2019

This was released in retry 0.8.1.0. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.