retry: Add `StandardRetryPolicy` and `standard_policy` mod #698

LucioFranco · 2022-10-07T14:22:37Z

This PR adds a new standard_policy module within the retry module that provides a batteries included policy to be used with the retry middleware. The policy combines the Budget type and generic backoff utlities from the backoff module to provide an easy to use policy with good defaults.

This PR also includes a StandardRetryPolicyBuilder as well as two new traits IsRetryable and CloneRequest. These each have blanket impls for closures. The reason that this implementation breaks these out into two different traits is to allow tower-http to provide a custom CloneRequest implementation that will be able to clone some sort of ReplayBody and let the user pass in the retry decision implementation.

Ref #682

This PR adds a new `standard_policy` module within the retry module that provides a batteries included policy to be used with the retry middleware. The policy combines the `Budget` type and generic backoff utlities from the `backoff` module to provide an easy to use policy with good defaults. This PR also includes a `StandardRetryPolicyBuilder` as well as two new traits `IsRetryable` and `CloneRequest`. These each have blanket impls for closures. The reason that this implementation breaks these out into two different traits is to allow `tower-http` to provide a custom `CloneRequest` implementation that will be able to clone some sort of `ReplayBody` and let the user pass in the retry decision implementation. Ref #682

davidpdrsn

LGTM

hawkw · 2022-10-13T20:16:35Z

tower/src/retry/mod.rs


 pub mod backoff;
 pub mod budget;
 pub mod future;
 mod layer;
 mod policy;
+pub mod standard_policy;


I'm not sure how I feel about calling this "standard policy"; I might prefer a name that actually describes what the policy does, rather than just saying it's "standard"...

tower/src/retry/standard_policy.rs

hawkw · 2022-10-13T20:32:43Z

tower/src/retry/standard_policy.rs

+/// A trait to determine if the request associated with the response should be
+/// retried by [`StandardRetryPolicy`].
+///
+/// # Closure
+///
+/// This trait provides a blanket implementation for a closure of the type
+/// `Fn(&mut Result<Res, E>) -> bool + Send + Sync + 'static`.
+pub trait IsRetryable<Res, E>: Send + Sync + 'static {
+    /// Return `true` if the request associated with the response should be
+    /// retried and `false` if it should not be retried.
+    fn is_retryalbe(&self, response: &mut Result<Res, E>) -> bool;


Hmm, it seems like this trait should be able to consider the request as well as the response in determining whether a result is retryable. For example, some HTTP request methods are idempotent, and others are non-idempotent; and non-idempotent requests should generally not be retried. With the current implementation, the IsRetryable function can't determine that a given request method is never retryable (without some kind of unfortunate contortion to stuff metadata about the request into the response type, or something).

It seems like it would be good to allow the request type to be considered as part of making this decision, especially if we want the StandardPolicy to be usable for HTTP retries...

hawkw · 2022-10-13T20:33:26Z

tower/src/retry/standard_policy.rs

+/// A trait to clone a request for the [`StandardRetryPolicy`].
+///
+/// # Closure
+///
+/// This trait provides a blanket implementation for a closure of the type
+/// `Fn(&Req) -> Option<Req> + Send + Sync + 'static`.
+pub trait CloneRequest<Req>: Send + Sync + 'static {
+    /// Clone a request, if `None` is returned the request will not be retried.
+    fn clone_request(&self, request: &Req) -> Option<Req>;
+}


it seems like there probably ought to be an implementation of this that's impl CloneRequest<Req> where Req: Clone that just calls Clone. Perhaps that ought to be the default?

hawkw · 2022-10-13T20:35:37Z

tower/src/retry/standard_policy.rs

+//! - [`IsRetryable`] by default will always return `false`.
+//! - [`CloneRequest`] by default will always return `None`.


It seems kind of unfortunate to me that the default behavior for the batteries-included retry policy is to...never retry anything. In particular, defaulting to a CloneRequest implementation that always returns None feels a bit weird to me.

What do you think about an API where the default implementation expects that the request type implements Clone, and just calls its Clone method? That way, users whose requests implement Clone never have to think about the CloneRequest trait, and providing a CloneRequest implementation is only necessary if you want to retry !Clone requests?

tower/src/retry/mod.rs

hawkw · 2022-10-13T20:40:11Z

tower/src/retry/mod.rs

+//!
+//! The [`budget`] module contains utilities to reduce the amount of concurrent
+//! retries made by a tower middleware stack. The goal for this is to reduce
+//! congestive collapse when downstream systems degrade.


could maybe be nice to make "congestive collapse" a link to Wikipedia or something?

hawkw · 2022-10-13T20:40:51Z

tower/src/retry/mod.rs

+//! The [`standard_policy`] module contains a default retry [`Policy`] that can
+//! be used with the [`Retry`] middleware. For more information check the module
+//! docs for [`standard_policy`].
+//!
+//! The [`backoff`] module contains backoff utlities that can be used in a
+//! custom [`Policy`].
+//!
+//! The [`budget`] module contains utilities to reduce the amount of concurrent
+//! retries made by a tower middleware stack. The goal for this is to reduce
+//! congestive collapse when downstream systems degrade.


take it or leave it: it seems kind of like these paragraphs could make sense as a bulleted list?

hawkw · 2022-10-13T20:42:39Z

tower/src/retry/standard_policy.rs

+    }
+}
+
+/// A standard retry [`Policy`] that combines a retry budget and a backoff


maybe "retry budget" and "backoff" here should link to the respective modules?

hawkw · 2022-10-13T20:59:24Z

tower/src/retry/standard_policy.rs

+    is_retryable: Arc<dyn IsRetryable<Res, E>>,
+    clone_request: Arc<dyn CloneRequest<Req>>,


a little bit on the fence about whether always Arcing these is the right choice or not --- a question that's definitely come up in the past with other middleware that needs to clone a Fn value. for example, MapRequest is generic over a F: FnMut(...) + Clone, and MapResponse is generic over a FnOnce + Clone, rather than Arcing a Fn...

tower/tower/src/builder/mod.rs

Line 200 in 878b10f

F: FnMut(R1) -> R2 + Clone,

in this case, since the functions are always cloned into each request, it wouldn't make sense to allow FnMut + Clone, since the mutable state wouldn't be shared across instances of the closure. but, it would allow the use of function pointers (not closures) to avoid allocating an Arc and bumping two reference counts on every request...

also, maybe a microoptimization, but it could be worth considering combining the IsRetryable and CloneRequest impls into a single struct that's then Arced, so that we only increment/decrement one atomic ref count on each request, rather than two...but, OTTOH, we would probably have to either box them or make the struct generic, if we did that, and boxing them would mean two heap ptr derefs...:woman_shrugging:

Co-authored-by: Eliza Weisman <[email protected]>

LucioFranco requested review from seanmonstar, olix0r, davidpdrsn and hawkw October 7, 2022 14:22

Fix tests

a3cdc6f

LucioFranco mentioned this pull request Oct 12, 2022

Retry middleware improvements #682

Open

19 tasks

Merge remote-tracking branch 'origin/master' into lucio/standard-policy2

88b7df9

LucioFranco force-pushed the lucio/standard-policy2 branch from 716f9d0 to 88b7df9 Compare October 12, 2022 15:12

Add tracing to retry feature

2d24ccb

davidpdrsn approved these changes Oct 13, 2022

View reviewed changes

hawkw reviewed Oct 13, 2022

View reviewed changes

LucioFranco and others added 2 commits October 27, 2022 11:22

Apply suggestions from code review

d97afa5

Co-authored-by: Eliza Weisman <[email protected]>

Fix rename

31e3326

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

retry: Add `StandardRetryPolicy` and `standard_policy` mod #698

retry: Add `StandardRetryPolicy` and `standard_policy` mod #698

LucioFranco commented Oct 7, 2022

davidpdrsn left a comment

hawkw Oct 13, 2022

hawkw Oct 13, 2022

hawkw Oct 13, 2022

hawkw Oct 13, 2022

hawkw Oct 13, 2022

hawkw Oct 13, 2022

hawkw Oct 13, 2022

hawkw Oct 13, 2022

hawkw Oct 13, 2022

		//! - [`IsRetryable`] by default will always return `false`.
		//! - [`CloneRequest`] by default will always return `None`.

		is_retryable: Arc<dyn IsRetryable<Res, E>>,
		clone_request: Arc<dyn CloneRequest<Req>>,

retry: Add StandardRetryPolicy and standard_policy mod #698

Are you sure you want to change the base?

retry: Add StandardRetryPolicy and standard_policy mod #698

Conversation

LucioFranco commented Oct 7, 2022

davidpdrsn left a 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

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: Add `StandardRetryPolicy` and `standard_policy` mod #698

retry: Add `StandardRetryPolicy` and `standard_policy` mod #698