retry: Change Policy to accept &mut self

[LucioFranco]

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]

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.

[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

[olix0r]

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]

@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]

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

[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]

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?

[hawkw]

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

[hawkw]

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

[LucioFranco]

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.