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.

Sign up for GitHub

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

retry: Change Policy to accept &mut self #681

Merged
merged 10 commits into from
Aug 23, 2022
Merged

retry: Change Policy to accept &mut self #681

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a859b22
retry: Change `Policy` to accept `&mut self`
LucioFranco Aug 3, 2022
416e323
Remove `Unpin` requirement for policy
LucioFranco Aug 10, 2022
f028e65
Improve docs
LucioFranco Aug 12, 2022
c047a8c
Relax service bounds
LucioFranco Aug 12, 2022
c4f45be
Merge remote-tracking branch 'origin/master' into lucio/update-policy
LucioFranco Aug 12, 2022
a930936
Update doc tests for Budget
LucioFranco Aug 15, 2022
a12f517
Merge branch 'master' into lucio/update-policy
LucioFranco Aug 15, 2022
b98f538
Apply suggestions from code review
LucioFranco Aug 22, 2022
e679d5d
Rename Checking to Waiting
LucioFranco Aug 22, 2022
e0aa369
style
LucioFranco Aug 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions tower/src/retry/future.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,8 @@ where
}
}
StateProj::Checking { checking } => {
this.retry
.as_mut()
.project()
.policy
.set(ready!(checking.poll(cx)));
ready!(checking.poll(cx));
LucioFranco marked this conversation as resolved.
Show resolved Hide resolved

this.state.set(State::Retrying);
}
StateProj::Retrying => {
Expand Down
1 change: 0 additions & 1 deletion tower/src/retry/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ pin_project! {
/// A [`Policy`] classifies what is a "failed" response.
#[derive(Clone, Debug)]
pub struct Retry<P, S> {
#[pin]
policy: P,
service: S,
}
Expand Down
21 changes: 13 additions & 8 deletions tower/src/retry/policy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ use std::future::Future;
/// struct Attempts(usize);
///
/// impl<E> Policy<Req, Res, E> for Attempts {
/// type Future = future::Ready<Self>;
/// type Future = future::Ready<()>;
///
/// fn retry(&self, req: &mut Req, result: &mut Result<Res, E>) -> Option<Self::Future> {
/// fn retry(&mut self, req: &mut Req, result: &mut Result<Res, E>) -> Option<Self::Future> {
/// match result {
/// Ok(_) => {
/// // Treat all `Response`s as success,
Expand All @@ -28,7 +28,8 @@ use std::future::Future;
/// // But we limit the number of attempts...
/// if self.0 > 0 {
/// // Try again!
/// Some(future::ready(Attempts(self.0 - 1)))
/// self.0 -= 1;
/// Some(future::ready(()))
/// } else {
/// // Used all our attempts, no retry...
/// None
Expand All @@ -37,14 +38,14 @@ use std::future::Future;
/// }
/// }
///
/// fn clone_request(&self, req: &Req) -> Option<Req> {
/// fn clone_request(&mut self, req: &Req) -> Option<Req> {
/// Some(req.clone())
/// }
/// }
/// ```
pub trait Policy<Req, Res, E>: Sized {
pub trait Policy<Req, Res, E> {
/// The [`Future`] type returned by [`Policy::retry`].
type Future: Future<Output = Self>;
type Future: Future<Output = ()>;
Comment on lines 47 to +48
Copy link
Member

Choose a reason for hiding this comment

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

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...?

Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


/// Check the policy if a certain request should be retried.
///
LucioFranco marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -77,10 +78,14 @@ pub trait Policy<Req, Res, E>: Sized {
///
/// [`Service::Response`]: crate::Service::Response
/// [`Service::Error`]: crate::Service::Error
fn retry(&self, req: &mut Req, result: &mut Result<Res, E>) -> Option<Self::Future>;
fn retry(&mut self, req: &mut Req, result: &mut Result<Res, E>) -> Option<Self::Future>;

/// Tries to clone a request before being passed to the inner service.
///
/// If the request cannot be cloned, return [`None`].
fn clone_request(&self, req: &Req) -> Option<Req>;
fn clone_request(&mut self, req: &Req) -> Option<Req>;
}

// Ensure `Policy` is object safe
#[cfg(test)]
fn _obj_safe(_: Box<dyn Policy<(), (), (), Future = futures::future::Ready<()>>>) {}
6 changes: 3 additions & 3 deletions tower/tests/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,13 @@ where
Req: Clone,
E: Into<Box<dyn std::error::Error + Send + Sync + 'static>>,
{
type Future = Ready<Self>;
type Future = Ready<()>;

fn retry(&self, _req: &mut Req, _result: &mut Result<Res, E>) -> Option<Self::Future> {
fn retry(&mut self, _req: &mut Req, _result: &mut Result<Res, E>) -> Option<Self::Future> {
None
}

fn clone_request(&self, req: &Req) -> Option<Req> {
fn clone_request(&mut self, req: &Req) -> Option<Req> {
Some(req.clone())
}
}
42 changes: 21 additions & 21 deletions tower/tests/retry/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,16 +122,16 @@ type Handle = mock::Handle<Req, Res>;
struct RetryErrors;

impl Policy<Req, Res, Error> for RetryErrors {
type Future = future::Ready<Self>;
fn retry(&self, _: &mut Req, result: &mut Result<Res, Error>) -> Option<Self::Future> {
type Future = future::Ready<()>;
fn retry(&mut self, _: &mut Req, result: &mut Result<Res, Error>) -> Option<Self::Future> {
if result.is_err() {
Some(future::ready(RetryErrors))
Some(future::ready(()))
} else {
None
}
}

fn clone_request(&self, req: &Req) -> Option<Req> {
fn clone_request(&mut self, req: &Req) -> Option<Req> {
Some(*req)
}
}
Expand All @@ -140,16 +140,17 @@ impl Policy<Req, Res, Error> for RetryErrors {
struct Limit(usize);

impl Policy<Req, Res, Error> for Limit {
type Future = future::Ready<Self>;
fn retry(&self, _: &mut Req, result: &mut Result<Res, Error>) -> Option<Self::Future> {
type Future = future::Ready<()>;
fn retry(&mut self, _: &mut Req, result: &mut Result<Res, Error>) -> Option<Self::Future> {
if result.is_err() && self.0 > 0 {
Some(future::ready(Limit(self.0 - 1)))
self.0 -= 1;
Some(future::ready(()))
} else {
None
}
}

fn clone_request(&self, req: &Req) -> Option<Req> {
fn clone_request(&mut self, req: &Req) -> Option<Req> {
Some(*req)
}
}
Expand All @@ -158,18 +159,18 @@ impl Policy<Req, Res, Error> for Limit {
struct UnlessErr(InnerError);

impl Policy<Req, Res, Error> for UnlessErr {
type Future = future::Ready<Self>;
fn retry(&self, _: &mut Req, result: &mut Result<Res, Error>) -> Option<Self::Future> {
type Future = future::Ready<()>;
fn retry(&mut self, _: &mut Req, result: &mut Result<Res, Error>) -> Option<Self::Future> {
result.as_ref().err().and_then(|err| {
if err.to_string() != self.0 {
Some(future::ready(self.clone()))
Some(future::ready(()))
} else {
None
}
})
}

fn clone_request(&self, req: &Req) -> Option<Req> {
fn clone_request(&mut self, req: &Req) -> Option<Req> {
Some(*req)
}
}
Expand All @@ -178,12 +179,12 @@ impl Policy<Req, Res, Error> for UnlessErr {
struct CannotClone;

impl Policy<Req, Res, Error> for CannotClone {
type Future = future::Ready<Self>;
fn retry(&self, _: &mut Req, _: &mut Result<Res, Error>) -> Option<Self::Future> {
type Future = future::Ready<()>;
fn retry(&mut self, _: &mut Req, _: &mut Result<Res, Error>) -> Option<Self::Future> {
unreachable!("retry cannot be called since request isn't cloned");
}

fn clone_request(&self, _req: &Req) -> Option<Req> {
fn clone_request(&mut self, _req: &Req) -> Option<Req> {
None
}
}
Expand All @@ -199,21 +200,20 @@ impl Policy<Req, Res, Error> for MutatingPolicy
where
Error: From<&'static str>,
{
type Future = future::Ready<Self>;
type Future = future::Ready<()>;

fn retry(&self, req: &mut Req, result: &mut Result<Res, Error>) -> Option<Self::Future> {
fn retry(&mut self, req: &mut Req, result: &mut Result<Res, Error>) -> Option<Self::Future> {
if self.remaining == 0 {
*result = Err("out of retries".into());
None
} else {
*req = "retrying";
Some(future::ready(MutatingPolicy {
remaining: self.remaining - 1,
}))
self.remaining -= 1;
Some(future::ready(()))
}
}

fn clone_request(&self, req: &Req) -> Option<Req> {
fn clone_request(&mut self, req: &Req) -> Option<Req> {
Some(*req)
}
}
Expand Down