tower icon indicating copy to clipboard operation
tower copied to clipboard

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

Open LucioFranco opened this issue 2 years ago • 4 comments

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

LucioFranco avatar Aug 03 '22 22:08 LucioFranco

cc @olix0r

LucioFranco avatar Aug 09 '22 16:08 LucioFranco

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?

olix0r avatar Aug 12 '22 15:08 olix0r

@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 avatar Aug 12 '22 16:08 LucioFranco

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 avatar Aug 12 '22 16:08 LucioFranco

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

LucioFranco avatar Aug 15 '22 17:08 LucioFranco