reqwest-middleware icon indicating copy to clipboard operation
reqwest-middleware copied to clipboard

`build_with_total_retry_duration` examples?

Open bbaldino opened this issue 1 year ago • 4 comments

Motivations

I know there have been a couple issues filed about confusion regarding ExponentialBackoffTimed--I had the same confusion myself, which is how I came across those issues. I think I understand the intent of the fix that made this change, but I'm wondering what an intended use of this API would look like (I searched around github, but couldn't find any examples of how others are using it).

Mainly: it feels awkward to have to create the policy each time a request is made. To me the appeal of middleware was being able to bake this sort of retry into the client such that code that just wants to make the request wouldn't have to worry about it...but the way this feature is set up it looks like calling code would need to create its own ClientWithMiddleware each time it creates a request to be able to pass for_task_started_at to the retry policy each time? Or is there another pattern that I'm missing?

Solution

It would be great to see some examples in the README of the intended usage of this API

Alternatives

n/a

Additional context

n/a

bbaldino avatar Oct 04 '23 04:10 bbaldino

I was also wondering: it looks like RetryTransientMiddleware has state to track the number of past retries, and RetryPolicy has specific support for the number of past retries...could RetryPolicy also take in the task start time and RetryTransientMiddleware track the task start time to enable ExponentialBackoffTimed to be able to implement RetryPolicy directly like ExponentialBackoff does?

If that's reasonable I could take a shot at implementing it.

bbaldino avatar Oct 04 '23 05:10 bbaldino

Hey! Sorry for the delay responding. You're right, RetryTransientMiddleware should keep track of the task start time, I shouldn't have closed these other issues.

I think adding the task start time to RetryPolicy::should_retry is a reasonable change, if you're still up for implementing it we're open for PRs 🙇

tl-rodrigo-gryzinski avatar Dec 11 '23 15:12 tl-rodrigo-gryzinski

Thanks...we ended up working around this so haven't looked at it in a while, but if I can find some time I'll come back and take a shot at it.

bbaldino avatar Jan 09 '24 18:01 bbaldino

I opened https://github.com/TrueLayer/retry-policies/pull/16 and https://github.com/TrueLayer/reqwest-middleware/pull/113 for this.

bbaldino avatar Jan 25 '24 22:01 bbaldino