improve flexiblity of retry policy #584

rcoh · 2021-04-30T20:35:12Z

retry::Policy is an effective way to expressing retries, however, there two use cases that
as it stands today, cannot be expressed:

Altering the final response (eg. to record the fact that you ran out of retries)
Altering the request (eg. to set a header to the server indicating that this request is a retry)

(Technically the second is possible with clone_request, but it's a little unclear which request would actually get sent).

This change implements what I think is pretty close to the minimal update to make this possible, namely, req
and Res both become mutable references. This enables policies to mutate them during execution & enables both of the
use cases above without complicating the "simple path" callers who don't need this behavior.

This is a breaking change. However, the fixes are only a couple of &mut and potentially a call to as_ref().

I've also considered (and would be totally happy) with an API that "mapped" over req & res, taking and returning ownership. This may actually even be slightly more flexible. However, I didn't want to leak this detail onto all users.

retry::Policy is an effective way to expressing retries, however, there two use cases that as it stands today, cannot be expressed: - Altering the final response (eg. to record the fact that you ran out of retries) - Altering the request (eg. to set a header to the server indicating that this request is a retry) (Technically the second is possible with `clone_request`, but it's a little unclear _which_ request would actually get sent). This change implements what I think is pretty close to the minimal update to make this possible, namely, `req` and `Res` both become mutable references. This enables policies to mutate them during execution & enables both of the use cases above without complicating the "simple path" callers who don't need this behavior. **This is a breaking change.** However, the fixes are only a couple of `&mut` and potentially a call to `as_ref()`.

davidpdrsn

I have never used this middleware in practice but to me the change looks sensible. Though I'm wondering if someone else with more experience using Retry has run into other problems that could be solved, should we decide to make a breaking change. cc @hawkw @olix0r @LucioFranco @seanmonstar

Had one small suggestion/question.

I've also considered (and would be totally happy) with an API that "mapped" over req & res, taking and returning ownership. This may actually even be slightly more flexible. However, I didn't want to leak this detail onto all users.

To return a new request/response the future returned by Policy::retry would have to change as well which might be too much of a change.

davidpdrsn · 2021-05-02T12:10:03Z

tower/src/retry/policy.rs

@@ -58,7 +58,7 @@ pub trait Policy<Req, Res, E>: Sized {
    ///
    /// [`Service::Response`]: crate::Service::Response
    /// [`Service::Error`]: crate::Service::Error
-    fn retry(&self, req: &Req, result: Result<&Res, &E>) -> Option<Self::Future>;
+    fn retry(&self, req: &mut Req, result: &mut Result<Res, E>) -> Option<Self::Future>;


I guess getting a &mut Result<Res, E> allows users to change Err(_) into Ok(_) which might be kinda odd. Not sure. Feels like getting Result<&mut Res, &mut E> would fit the use-cases as well but not allow changing the Result variant.

Oh yeah, that is a really good point david, what do you think about that @rcoh ?

comment wasn't threaded because of email:

The entire point is to change the variant (at least for me). Because there
are "success" cases the actually need to be retries and when I run out of
retries I need to change it to failure

rcoh · 2021-05-02T14:49:55Z

The entire point is to change the variant (at least for me). Because there are "success" cases the actually need to be retries and when I run out of retries I need to change it to failure

…

On Sun, May 2, 2021, 8:13 AM David Pedersen ***@***.***> wrote: ***@***.**** commented on this pull request. I have never used this middleware in practice but to me the change looks sensible. Though I'm wondering if someone else with more experience using Retry has run into other problems that could be solved, should we decide to make a breaking change. cc @hawkw <https://github.com/hawkw> @olix0r <https://github.com/olix0r> @LucioFranco <https://github.com/LucioFranco> @seanmonstar <https://github.com/seanmonstar> Had one small suggestion/question. I've also considered (and would be totally happy) with an API that "mapped" over req & res, taking and returning ownership. This may actually even be slightly more flexible. However, I didn't want to leak this detail onto all users. To return a new request/response the future returned by Policy::retry would have to change as well which might be too much of a change. ------------------------------ In tower/src/retry/policy.rs <#584 (comment)>: > @@ -58,7 +58,7 @@ pub trait Policy<Req, Res, E>: Sized { /// /// [`Service::Response`]: crate::Service::Response /// [`Service::Error`]: crate::Service::Error - fn retry(&self, req: &Req, result: Result<&Res, &E>) -> Option<Self::Future>; + fn retry(&self, req: &mut Req, result: &mut Result<Res, E>) -> Option<Self::Future>; I guess getting a &mut Result<Res, E> allows users to change Err(_) into Ok(_) which might be kinda odd. Not sure. Feels like getting Result<&mut Res, &mut E> would fit the use-cases as well but not allow changing the Result variant. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#584 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADYKZZKITAKKYCN6H47VJ3TLU6YBANCNFSM435NQWHQ> .

davidpdrsn · 2021-05-04T20:06:06Z

Alright cool. That does make sense.

I kinda feel like if we are going to make a breaking change we should take care to design something that we are happy with. That might be this or it might be bigger changes. I haven't used Retry personally so I don't know which it is. There was some discussion about other potential changes to Retry here.

LucioFranco · 2021-05-07T15:22:50Z

@rcoh maybe I am forgetting how this works but can't you return an error in the returned service future?

rcoh · 2021-05-07T15:28:08Z

@LucioFranco you have a mut ref to the result, so you can *res = Err(...) (there's a UT in the PR that does this)

hawkw · 2021-05-19T21:21:50Z

i actually just ran into a use-case for a mutable request in clone_request. i was trying to implement a retry policy where an HTTP request body is buffered lazily as the first request's body is sent, rather than aggregating the whole body prior to sending the initial request.

the approach i was going to take was storing the buffered body on the initial request, and then sending the buffer on a oneshot channel when the first request is dropped so that the potential retry can take ownership of it. however, there isn't currently any way to get the receiver side of the oneshot out of the initial request, without introducing a mutex or something for this purpose. making the request mutable in clone_request would very make this kind of thing much easier, so i think we should definitely make this change!

rcoh · 2021-05-19T21:28:12Z

@hawkw just to clarify, in this change the request becomes mutable in the retry method; I thought it was a little unclear from a user perspective which request was which if you were to mutate it in clone_request() (basically, is the cloned request sent first or is the original sent first?)

Which is why I made it mutatable in retry because then it seemed more obvious that the request you just mutated would get sent next. I think that should be equivalent with your try_clone use case, but we could also make it mutable in both cases?

hawkw · 2021-05-19T21:34:36Z

@hawkw just to clarify, in this change the request becomes mutable in the retry method; I thought it was a little unclear from a user perspective which request was which if you were to mutate it in clone_request() (basically, is the cloned request sent first or is the original sent first?)

Which is why I made it mutatable in retry because then it seemed more obvious that the request you just mutated would get sent next. I think that should be equivalent with your try_clone use case, but we could also make it mutable in both cases?

oh, i misread the PR description, whoops! i'm going to just go ahead and try to implement a lazily buffered request body against this branch and report back? i think it may still be possible without also touching clone_request, but we'll see...

another option would be to make the Policy itself mutable in both clone_request() and retry(), which would also give you an avenue for sharing state between the first and second request...

hawkw · 2022-03-31T18:10:11Z

It would be really nice to get this change into 0.5 (which we're currently working on). @rcoh, are you interested in updating this branch to build with the latest master, or should I?

rcoh · 2022-03-31T20:47:09Z

I can do the merge tomorrow assuming there's nothing too problematic

…y-policy

…exible-retry-policy

rcoh · 2022-04-01T22:08:48Z

merge is done. I also added another section of docs about the mutable fields—I assume you'll squash merge, but if not let me know and I can cleanup history

hawkw

some minor documentation suggestions, hope they're helpful!

tower/src/retry/policy.rs

hawkw · 2022-04-08T16:19:47Z

tower/src/retry/policy.rs

+    ///
+    /// The policy MAY chose to mutate the result. This can enable the retry policy to convert a failure
+    /// into a success and vice versa. For example, if the policy is used to poll while waiting for a state
+    /// change, you can switch the result to failure such that running out of retries returns a specific failure.


nit: in general, we prefer to avoid second-person ('you') language in docs:

Suggested change

/// change, you can switch the result to failure such that running out of retries returns a specific failure.

/// change, the policy can switch the result to failure such that running out of retries returns a specific failure.

tower/src/retry/policy.rs

hawkw · 2022-04-08T16:21:21Z

I assume you'll squash merge, but if not let me know and I can cleanup history

Yeah, that's right, we generally merge PRs by squashing, so you don't have to worry about it! :)

- Wrap docs to 80 characters - Small doc tweaks / rewrites to clarify & remove `you`

…exible-retry-policy

davidpdrsn added the A-retry Area: The tower "retry" middleware label May 1, 2021

davidpdrsn reviewed May 2, 2021

View reviewed changes

LucioFranco requested review from seanmonstar and hawkw May 7, 2021 14:15

hawkw added this to the 0.5 milestone May 7, 2021

Merge branch 'master' into flexible-retry-policy

b585e5e

LucioFranco approved these changes Mar 31, 2022

View reviewed changes

rcoh added 2 commits April 1, 2022 15:04

Merge branch 'master' of github.com:tower-rs/tower into flexible-retr…

9b2aaf1

…y-policy

Merge branch 'flexible-retry-policy' of github.com:rcoh/tower into fl…

890f895

…exible-retry-policy

Update changelog

72a063b

LucioFranco requested a review from davidpdrsn April 4, 2022 19:34

Merge branch 'master' into flexible-retry-policy

cf281c8

hawkw reviewed Apr 8, 2022

View reviewed changes

rcoh and others added 3 commits April 8, 2022 16:42

doc updates

ec17eb4

- Wrap docs to 80 characters - Small doc tweaks / rewrites to clarify & remove `you`

Merge branch 'flexible-retry-policy' of github.com:rcoh/tower into fl…

b8538bf

…exible-retry-policy

Merge branch 'master' into flexible-retry-policy

9a71523

LucioFranco merged commit 19c1a1d into tower-rs:master Aug 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve flexiblity of retry policy #584

improve flexiblity of retry policy #584

rcoh commented Apr 30, 2021 •

edited

Loading

davidpdrsn left a comment

davidpdrsn May 2, 2021

LucioFranco May 7, 2021

rcoh May 7, 2021

rcoh commented May 2, 2021 via email

davidpdrsn commented May 4, 2021

LucioFranco commented May 7, 2021

rcoh commented May 7, 2021 •

edited

Loading

hawkw commented May 19, 2021

rcoh commented May 19, 2021

hawkw commented May 19, 2021

hawkw commented Mar 31, 2022

rcoh commented Mar 31, 2022

rcoh commented Apr 1, 2022 •

edited

Loading

hawkw left a comment

hawkw Apr 8, 2022

hawkw commented Apr 8, 2022

	/// change, you can switch the result to failure such that running out of retries returns a specific failure.
	/// change, the policy can switch the result to failure such that running out of retries returns a specific failure.

improve flexiblity of retry policy #584

improve flexiblity of retry policy #584

Conversation

rcoh commented Apr 30, 2021 • edited Loading

davidpdrsn left a comment

Choose a reason for hiding this comment

davidpdrsn May 2, 2021

Choose a reason for hiding this comment

LucioFranco May 7, 2021

Choose a reason for hiding this comment

rcoh May 7, 2021

Choose a reason for hiding this comment

rcoh commented May 2, 2021 via email

davidpdrsn commented May 4, 2021

LucioFranco commented May 7, 2021

rcoh commented May 7, 2021 • edited Loading

hawkw commented May 19, 2021

rcoh commented May 19, 2021

hawkw commented May 19, 2021

hawkw commented Mar 31, 2022

rcoh commented Mar 31, 2022

rcoh commented Apr 1, 2022 • edited Loading

hawkw left a comment

Choose a reason for hiding this comment

hawkw Apr 8, 2022

Choose a reason for hiding this comment

hawkw commented Apr 8, 2022

rcoh commented Apr 30, 2021 •

edited

Loading

rcoh commented May 7, 2021 •

edited

Loading

rcoh commented Apr 1, 2022 •

edited

Loading