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

improve flexiblity of retry policy #584

Merged
merged 9 commits into from
Aug 1, 2022
4 changes: 2 additions & 2 deletions tower/src/retry/future.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,9 @@ where

loop {
match this.state.as_mut().project() {
StateProj::Called(future) => {
StateProj::Called { future } => {
let mut result = ready!(future.poll(cx));
if let Some(ref mut req) = this.request {
if let Some(req) = &mut this.request {
match this.retry.policy.retry(req, &mut result) {
Some(checking) => {
this.state.set(State::Checking { checking });
Expand Down
15 changes: 15 additions & 0 deletions tower/src/retry/policy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,21 @@ pub trait Policy<Req, Res, E>: Sized {
/// If the request *should* be retried, return `Some` future of a new
/// policy that would apply for the next request attempt.
///
/// ## Mutating Requests
///
/// The policy MAY chose to mutate the `req`: if the request is mutated, the mutated request
/// will be sent to the inner service in the next retry. This can be helpful for use cases like
/// tracking the retry count in a header.
///
/// ## Mutating Results
///
/// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

///
/// You can also record metadata on the request to include information about the number of retries required
/// or to record that a failure failed after exhausting all retries.
///
/// [`Service::Response`]: crate::Service::Response
/// [`Service::Error`]: crate::Service::Error
fn retry(&self, req: &mut Req, result: &mut Result<Res, E>) -> Option<Self::Future>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand Down
7 changes: 3 additions & 4 deletions tower/tests/retry/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,17 +94,14 @@ async fn success_with_cannot_clone() {
async fn retry_mutating_policy() {
let _t = support::trace_init();

// Even though the request couldn't be cloned, if the first request succeeds,
// it should succeed overall.
let (mut service, mut handle) = new_service(MutatingPolicy { remaining: 2 });

assert_ready_ok!(service.poll_ready());
let mut fut = task::spawn(service.call("hello"));

assert_request_eq!(handle, "hello").send_response("world");
assert_pending!(fut.poll());
// the policy alters the request. in real life, this might be setting
// a header
// the policy alters the request. in real life, this might be setting a header
assert_request_eq!(handle, "retrying").send_response("world");

assert_pending!(fut.poll());
Expand Down Expand Up @@ -191,6 +188,8 @@ impl Policy<Req, Res, Error> for CannotClone {
}
}

/// Test policy that changes the request to `retrying` during retries and the result to `"out of retries"`
/// when retries are exhausted.
#[derive(Clone)]
struct MutatingPolicy {
remaining: usize,
Expand Down