retry: Change `Policy` to accept `&mut self` #681

LucioFranco · 2022-08-03T22:48:40Z

This changes the Policy trait in the retry layer to accept &mut self instead of &self and changes the output type of the returned
future to (). The motivation for this change is to simplify
the trait a bit. By the trait methods having mutable references it means
that for each retry request session you can mutate the local policy.
This is because the policy is cloned for each individual request that
arrives into the retry middleware. In addition, this allows the Policy
trait to be object safe.

cc @rcoh

This changes the `Policy` trait in the `retry` layer to accept `&mut self` instead of `&self` and changes the output type of the returned future to `()`. The motivation for this change is to simplify the trait a bit. By the trait methods having mutable references it means that for each retry request session you can mutate the local policy. This is because the policy is cloned for each individual request that arrives into the retry middleware. In addition, this allows the `Policy` trait to be object safe.

LucioFranco · 2022-08-09T16:18:20Z

cc @olix0r

seanmonstar

I think these changes are likely the right way to go. I'd prefer to get r+s from @olix0r and @rcoh, since they have impls that would need to be changed.

tower/src/retry/future.rs

rcoh

I think the docs will need a close pass to make sure they all get updated, otherwise LGTM! the one thing I might consider is making the returned type T=() I'm thinking about token bucket use cases where the policy might want to take and then return a token or something...but it's not immediately obvious to me how that would get wired

tower/src/retry/policy.rs

olix0r · 2022-08-12T15:42:52Z

I'm a little unclear on this:

By the trait methods having mutable references it means
that for each retry request session you can mutate the local policy.
This is because the policy is cloned for each individual request that
arrives into the retry middleware

So, we clone the policy for each request AND it is mutable? How does that work? If it's cloned for each request, we'd need an Arc<Mutex<..>> for state to be held across all clones? Or is the mutability only intended to support mutating a single cloned replica?

LucioFranco · 2022-08-12T16:06:53Z

@olix0r By each request I mean request session, maybe that is a better way to word it. Aka we only clone the policy once iirc. So if you want data to be shared across all instances of the retry middleware then you'd need to Arc<Mutex<..>.

LucioFranco · 2022-08-12T16:16:51Z

tower/src/retry/mod.rs

+    /// of `'static` futures. To easily add `Clone` to your service you can
+    /// use the `Buffer` middleware.
+    ///
+    /// The `Policy` is also required to implement `Clone`. This middleware will


Hopefully this and the removal of the Clone and the relaxation of the bounds on ResponseFuture make this more clear.

LucioFranco · 2022-08-12T16:17:46Z

the one thing I might consider is making the returned type T=() I'm thinking about token bucket use cases where the policy might want to take and then return a token or something...but it's not immediately obvious to me how that would get wired

@rcoh I would imagine you can do anything with &mut self? and Arc if you need it shared.

LucioFranco · 2022-08-15T17:29:08Z

Ok this is ready for another round of reviews, CI is only failing due to a rustc beta/nightly bug.

hawkw

overall, I think this new design makes sense; — it's certainly simpler than the previous one. my one big question is whether there's a valid use case for generating the next Policy itself asynchronously that can't be satisfied with another approach. i don't really think there is, but perhaps others have input on that?

tower/src/retry/mod.rs

hawkw · 2022-08-19T19:24:58Z

tower/src/retry/policy.rs

    /// The [`Future`] type returned by [`Policy::retry`].
-    type Future: Future<Output = Self>;
+    type Future: Future<Output = ()>;


changing this to return a Future<Output = ()> and duplicating the Policy using Clone rather than having the future return a new policy definitely simplifies things, but it occurs to me that asynchronous work can no longer be performed in order to update the Policy. i can't immediately come up with a reason that it would be necessary to generate the next Policy asynchronously, but it seems like it could be worth thinking about before we commit to this design...?

thinking about it, i suppose that if an implementation needed to modify the Policy after the future's completion, it could always clone an Arced shared state into its Future...this may introduce some overhead over the current approach, but I don't really think that use case is common enough that it's particularly important to worry about...

Yeah, I don't think that use case is something we should support with that level and anyways taking a lock on an uncontested arc/mutex should be very cheap and much cheaper than request io etc.

tower/src/retry/policy.rs

tower/src/retry/future.rs

Co-authored-by: Eliza Weisman <eliza@buoyant.io>

this commit updates our tower dependency from 0.4 to 0.5. note that this commit does not affect the `tower-service` and `tower-layer` crates, reëxported by `tower` itself. the `Service<T>` trait and the closely related `Layer<S>` trait have not been changed. the `tower` crate's utilities have changed in various ways, some of particular note for the linkerd2 proxy. see these items, excerpted from the tower changelog: - **retry**: **Breaking Change** `retry::Policy::retry` now accepts `&mut Req` and `&mut Res` instead of the previous mutable versions. This increases the flexibility of the retry policy. To update, update your method signature to include `mut` for both parameters. ([#584]) - **retry**: **Breaking Change** Change Policy to accept &mut self ([#681]) - **retry**: **Breaking Change** `Budget` is now a trait. This allows end-users to implement their own budget and bucket implementations. ([#703]) - **util**: **Breaking Change** `Either::A` and `Either::B` have been renamed `Either::Left` and `Either::Right`, respectively. ([#637]) - **util**: **Breaking Change** `Either` now requires its two services to have the same error type. ([#637]) - **util**: **Breaking Change** `Either` no longer implemenmts `Future`. ([#637]) - **buffer**: **Breaking Change** `Buffer<S, Request>` is now generic over `Buffer<Request, S::Future>.` ([#654]) see: * <tower-rs/tower#584> * <tower-rs/tower#681> * <tower-rs/tower#703> * <tower-rs/tower#637> * <tower-rs/tower#654> the `Either` trait bounds are particularly impactful for us. because this runs counter to how we treat errors (skewing towards boxed errors, in general), we temporarily vendor a version of `Either` from the 0.4 release, whose variants have been renamed to match the 0.5 interface. updating to box the inner `A` and `B` services' errors, so we satiate the new `A::Error = B::Error` bounds, can be addressed as a follow-on. that's intentionally left as a separate change, due to the net size of our patchset between this branch and #3504. * <tower-rs/tower@v0.4.x...master> * <https://github.com/tower-rs/tower/blob/master/tower/CHANGELOG.md> this work is based upon #3504. for more information, see: * linkerd/linkerd2#8733 * #3504 Signed-off-by: katelyn martin <kate@buoyant.io> X-Ref: tower-rs/tower#815 X-Ref: tower-rs/tower#817 X-Ref: tower-rs/tower#818 X-Ref: tower-rs/tower#819

this commit updates our tower dependency from 0.4 to 0.5. note that this commit does not affect the `tower-service` and `tower-layer` crates, reëxported by `tower` itself. the `Service<T>` trait and the closely related `Layer<S>` trait have not been changed. the `tower` crate's utilities have changed in various ways, some of particular note for the linkerd2 proxy. see these items, excerpted from the tower changelog: - **retry**: **Breaking Change** `retry::Policy::retry` now accepts `&mut Req` and `&mut Res` instead of the previous mutable versions. This increases the flexibility of the retry policy. To update, update your method signature to include `mut` for both parameters. ([tower-rs/tower#584]) - **retry**: **Breaking Change** Change Policy to accept &mut self ([tower-rs/tower#681]) - **retry**: **Breaking Change** `Budget` is now a trait. This allows end-users to implement their own budget and bucket implementations. ([tower-rs/tower#703]) - **util**: **Breaking Change** `Either::A` and `Either::B` have been renamed `Either::Left` and `Either::Right`, respectively. ([tower-rs/tower#637]) - **util**: **Breaking Change** `Either` now requires its two services to have the same error type. ([tower-rs/tower#637]) - **util**: **Breaking Change** `Either` no longer implemenmts `Future`. ([tower-rs/tower#637]) - **buffer**: **Breaking Change** `Buffer<S, Request>` is now generic over `Buffer<Request, S::Future>.` ([tower-rs/tower#654]) see: * <tower-rs/tower#584> * <tower-rs/tower#681> * <tower-rs/tower#703> * <tower-rs/tower#637> * <tower-rs/tower#654> the `Either` trait bounds are particularly impactful for us. because this runs counter to how we treat errors (skewing towards boxed errors, in general), we temporarily vendor a version of `Either` from the 0.4 release, whose variants have been renamed to match the 0.5 interface. updating to box the inner `A` and `B` services' errors, so we satiate the new `A::Error = B::Error` bounds, can be addressed as a follow-on. that's intentionally left as a separate change, due to the net size of our patchset between this branch and #3504. * <tower-rs/tower@v0.4.x...master> * <https://github.com/tower-rs/tower/blob/master/tower/CHANGELOG.md> this work is based upon #3504. for more information, see: * linkerd/linkerd2#8733 * #3504 Signed-off-by: katelyn martin <kate@buoyant.io> X-Ref: tower-rs/tower#815 X-Ref: tower-rs/tower#817 X-Ref: tower-rs/tower#818 X-Ref: tower-rs/tower#819

this commit updates our tower dependency from 0.4 to 0.5. note that this commit does not affect the `tower-service` and `tower-layer` crates, reëxported by `tower` itself. the `Service<T>` trait and the closely related `Layer<S>` trait have not been changed. the `tower` crate's utilities have changed in various ways, some of particular note for the linkerd2 proxy. see these items, excerpted from the tower changelog: - **retry**: **Breaking Change** `retry::Policy::retry` now accepts `&mut Req` and `&mut Res` instead of the previous mutable versions. This increases the flexibility of the retry policy. To update, update your method signature to include `mut` for both parameters. ([tower-rs/tower#584]) - **retry**: **Breaking Change** Change Policy to accept &mut self ([tower-rs/tower#681]) - **retry**: **Breaking Change** `Budget` is now a trait. This allows end-users to implement the 9E88 ir own budget and bucket implementations. ([tower-rs/tower#703]) - **util**: **Breaking Change** `Either::A` and `Either::B` have been renamed `Either::Left` and `Either::Right`, respectively. ([tower-rs/tower#637]) - **util**: **Breaking Change** `Either` now requires its two services to have the same error type. ([tower-rs/tower#637]) - **util**: **Breaking Change** `Either` no longer implemenmts `Future`. ([tower-rs/tower#637]) - **buffer**: **Breaking Change** `Buffer<S, Request>` is now generic over `Buffer<Request, S::Future>.` ([tower-rs/tower#654]) see: * <tower-rs/tower#584> * <tower-rs/tower#681> * <tower-rs/tower#703> * <tower-rs/tower#637> * <tower-rs/tower#654> the `Either` trait bounds are particularly impactful for us. because this runs counter to how we treat errors (skewing towards boxed errors, in general), we temporarily vendor a version of `Either` from the 0.4 release, whose variants have been renamed to match the 0.5 interface. updating to box the inner `A` and `B` services' errors, so we satiate the new `A::Error = B::Error` bounds, can be addressed as a follow-on. that's intentionally left as a separate change, due to the net size of our patchset between this branch and #3504. * <tower-rs/tower@v0.4.x...master> * <https://github.com/tower-rs/tower/blob/master/tower/CHANGELOG.md> this work is based upon #3504. for more information, see: * linkerd/linkerd2#8733 * #3504 Signed-off-by: katelyn martin <kate@buoyant.io> X-Ref: tower-rs/tower#815 X-Ref: tower-rs/tower#817 X-Ref: tower-rs/tower#818 X-Ref: tower-rs/tower#819

this commit updates our tower dependency from 0.4 to 0.5. note that this commit does not affect the `tower-service` and `tower-layer` crates, reëxported by `tower` itself. the `Service<T>` trait and the closely related `Layer<S>` trait have not been changed. the `tower` crate's utilities have changed in various ways, some of particular note for the linkerd2 proxy. see these items, excerpted from the tower changelog: - **retry**: **Breaking Change** `retry::Policy::retry` now accepts `&mut Req` and `&mut Res` instead of the previous mutable versions. This increases the flexibility of the retry policy. To update, update your method signature to include `mut` for both parameters. ([tower-rs/tower#584]) - **retry**: **Breaking Change** Change Policy to accept &mut self ([tower-rs/tower#681]) - **retry**: **Breaking Change** `Budget` is now a trait. This allows end-users to implement their own budget and bucket implementations. ([tower-rs/tower#703]) - **util**: **Breaking Change** `Either::A` and `Either::B` have been renamed `Either::Left` and `Either::Right`, respectively. ([tower-rs/tower#637]) - **util**: **Breaking Change** `Either` now requires its two services to have the same error type. ([tower-rs/tower#637]) - **util**: **Breaking Change** `Either` no longer implemenmts `Future`. ([tower-rs/tower#637]) - **buffer**: **Breaking Change** `Buffer<S, Request>` is now generic over `Buffer<Request, S::Future>.` ([tower-rs/tower#654]) see: * <tower-rs/tower#584> * <tower-rs/tower#681> * <tower-rs/tower#703> * <tower-rs/tower#637> * <tower-rs/tower#654> the `Either` trait bounds are particularly impactful for us. because this runs counter to how we treat errors (skewing towards boxed errors, in general), we temporarily vendor a version of `Either` from the 0.4 release, whose variants have been renamed to match the 0.5 interface. updating to box the inner `A` and `B` services' errors, so we satiate the new `A::Error = B::Error` bounds, can be addressed as a follow-on. that's intentionally left as a separate change, due to the net size of our patchset between this branch and #3504. * <tower-rs/tower@v0.4.x...master> * <https://github.com/tower-rs/tower/blob/master/tower/CHANGELOG.md> this work is based upon #3504. for more information, see: * linkerd/linkerd2#8733 * #3504 Signed-off-by: katelyn martin <kate@buoyant.io> X-Ref: tower-rs/tower#815 X-Ref: tower-rs/tower#817 X-Ref: tower-rs/tower#818 X-Ref: tower-rs/tower#819

* chore(deps)!: upgrade to tower 0.5 this commit updates our tower dependency from 0.4 to 0.5. note that this commit does not affect the `tower-service` and `tower-layer` crates, reëxported by `tower` itself. the `Service<T>` trait and the closely related `Layer<S>` trait have not been changed. the `tower` crate's utilities have changed in various ways, some of particular note A94B for the linkerd2 proxy. see these items, excerpted from the tower changelog: - **retry**: **Breaking Change** `retry::Policy::retry` now accepts `&mut Req` and `&mut Res` instead of the previous mutable versions. This increases the flexibility of the retry policy. To update, update your method signature to include `mut` for both parameters. ([tower-rs/tower#584]) - **retry**: **Breaking Change** Change Policy to accept &mut self ([tower-rs/tower#681]) - **retry**: **Breaking Change** `Budget` is now a trait. This allows end-users to implement their own budget and bucket implementations. ([tower-rs/tower#703]) - **util**: **Breaking Change** `Either::A` and `Either::B` have been renamed `Either::Left` and `Either::Right`, respectively. ([tower-rs/tower#637]) - **util**: **Breaking Change** `Either` now requires its two services to have the same error type. ([tower-rs/tower#637]) - **util**: **Breaking Change** `Either` no longer implemenmts `Future`. ([tower-rs/tower#637]) - **buffer**: **Breaking Change** `Buffer<S, Request>` is now generic over `Buffer<Request, S::Future>.` ([tower-rs/tower#654]) see: * <tower-rs/tower#584> * <tower-rs/tower#681> * <tower-rs/tower#703> * <tower-rs/tower#637> * <tower-rs/tower#654> the `Either` trait bounds are particularly impactful for us. because this runs counter to how we treat errors (skewing towards boxed errors, in general), we temporarily vendor a version of `Either` from the 0.4 release, whose variants have been renamed to match the 0.5 interface. updating to box the inner `A` and `B` services' errors, so we satiate the new `A::Error = B::Error` bounds, can be addressed as a follow-on. that's intentionally left as a separate change, due to the net size of our patchset between this branch and #3504. * <tower-rs/tower@v0.4.x...master> * <https://github.com/tower-rs/tower/blob/master/tower/CHANGELOG.md> this work is based upon #3504. for more information, see: * linkerd/linkerd2#8733 * #3504 Signed-off-by: katelyn martin <kate@buoyant.io> X-Ref: tower-rs/tower#815 X-Ref: tower-rs/tower#817 X-Ref: tower-rs/tower#818 X-Ref: tower-rs/tower#819 * fix(stack/loadshed): update test affected by tower-rs/tower#635 this commit updates a test that was affected by breaking changes in tower's `Buffer` middleware. see this excerpt from the description of that change: > I had to change some of the integration tests slightly as part of this > change. This is because the buffer implementation using semaphore > permits is _very subtly_ different from one using a bounded channel. In > the `Semaphore`-based implementation, a semaphore permit is stored in > the `Message` struct sent over the channel. This is so that the capacity > is used as long as the message is in flight. However, when the worker > task is processing a message that's been recieved from the channel, > the permit is still not dropped. Essentially, the one message actively > held by the worker task _also_ occupies one "slot" of capacity, so the > actual channel capacity is one less than the value passed to the > constructor, _once the first request has been sent to the worker_. The > bounded MPSC changed this behavior so that capacity is only occupied > while a request is actually in the channel, which broke some tests > that relied on the old (and technically wrong) behavior. bear particular attention to this: > The bounded MPSC changed this behavior so that capacity is only > occupied while a request is actually in the channel, which broke some > tests that relied on the old (and technically wrong) behavior. that pr adds an additional message to the channel in tests exercising the laod-shedding behavior, on account of the previous (incorrect) behavior. https://github.com/tower-rs/tower/pull/635/files#r797108274 this commit performs the same change for our corresponding test, adding an additional `ready()` call before we hit the buffer's limit. Signed-off-by: katelyn martin <kate@buoyant.io> * review: use vendored `Either` for consistency #3744 (comment) Signed-off-by: katelyn martin <kate@buoyant.io> --------- Signed-off-by: katelyn martin <kate@buoyant.io>

LucioFranco requested review from a team, seanmonstar and hawkw August 3, 2022 22:48

seanmonstar reviewed Aug 10, 2022

View reviewed changes

tower/src/retry/future.rs Outdated Show resolved Hide resolved

Remove Unpin requirement for policy

416e323

LucioFranco mentioned this pull request Aug 11, 2022

Retry middleware improvements #682

Open

19 tasks

rcoh approved these changes Aug 12, 2022

View reviewed changes

tower/src/retry/policy.rs Show resolved Hide resolved

LucioFranco added 2 commits August 12, 2022 12:15

Improve docs

f028e65

Relax service bounds

c047a8c

LucioFranco commented Aug 12, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into lucio/update-policy

c4f45be

LucioFranco requested a review from seanmonstar August 15, 2022 17:15

LucioFranco added 2 commits August 15, 2022 13:19

Update doc tests for Budget

a930936

Merge branch 'master' into lucio/update-policy

a12f517

hawkw reviewed Aug 19, 2022

View reviewed changes

LucioFranco and others added 3 commits August 22, 2022 13:06

Apply suggestions from code review

b98f538

Co-authored-by: Eliza Weisman <eliza@buoyant.io>

Rename Checking to Waiting

e679d5d

style

e0aa369

LucioFranco requested review from hawkw and davidpdrsn August 22, 2022 17:19

seanmonstar approved these changes Aug 23, 2022

View reviewed changes

LucioFranco merged commit aec7b8f into master Aug 23, 2022

LucioFranco deleted the lucio/update-policy branch August 23, 2022 18:30

Serendo mentioned this pull request Mar 13, 2025

feat(deps): update tower crate to v0.5.2 and fix partial retry logic vectordotdev/vector#22431

Open

10 tasks

cratelyn mentioned this pull request Mar 14, 2025

chore(deps): upgrade to tower 0.5 linkerd/linkerd2-proxy#3744

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

retry: Change `Policy` to accept `&mut self` #681

retry: Change `Policy` to accept `&mut self` #681

retry: Change Policy to accept &mut self #681

retry: Change Policy to accept &mut self #681

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retry: Change `Policy` to accept `&mut self` #681

retry: Change `Policy` to accept `&mut self` #681