Added a way to specify custom functions which decide whether a request should be retried or not

[Rutgerdj]

Added functionality that allows you to specify custom functions that will direct whether a request will be retried or not.

The implementation doesn't interfere with the already existing API.

Added

A RetryableStrategy trait, much like the SpanBackend trait for TracingMiddleware.

Default handlers are added for both success and error. (those are the results of splitting the original Retryable::from_reqwest_response function)

Added a function new_with_policy_and_strategy on RetryTransientMiddleware that takes a RetryableStrategy which will be used to decide whether a request should be retried.

[LukeMathWalker]

Wouldn't it be simpler to have a single method on the trait that takes a Result<Response, Error> instead of two separate methods?

[Rutgerdj]

Well it would be simpler to implement in the library. But for actual use in programs I think it will be useful to split them up. Especially because this allows you to only specify a handler for success responses and have the default one for errors

[tl-rodrigo-gryzinski]

Thanks for the contribution, well presented PR and this is nice functionality to add.

I agree with @LukeMathWalker about the single method, in my opinion that makes the API more intuitive and simpler. We don't need to give up the convenience of using specific functions for the success and error cases if we can provide a function to compose handlers of type (fn(Response) -> Option<Retryable>, fn(Error) -> Option<Retryable>) -> fn(Result<Response, Error>) -> Option<Retryable>, so clients could do with_retryable_strat(composite(my_result_strat, my_error_strat)).

Also I'd prefer to use a trait instead of a struct with fn fields, that's more idiomatic Rust, gives users the freedom to easily use state if needed and can provide better performance. The extra trait parameter to RetryTransientMiddleware doesn't need to be a breaking change, we can make it default to the current strategy.

Going back to the topic of convenience, I think there are a couple of useful combinators to add to RetryableStrategy:

• with_success<F: FnOnce(Request) -> Option<Retryable>>(self, F) -> impl RetryableStrategy, which would keep the error handler but use a different success handler.
• with_error(self, F) -> impl RetryableStrategy analogous to with_success to override the error handler.

The names are just illustrative, in case you have better suggestions. I didn't think through the trait bounds so they may need to be stricter as well.

[LukeMathWalker]

I don't think we need combinators, you can fallback on the default functionality for errors (or successes) using:

pub trait RetryableStrategy {
    fn handle(&self, outcome: Result<reqwest::Response, Error>) -> Option<Retryable>;
}

pub struct MyCustomRetriableStrategy { /* */ };

impl RetryableStrategy for MyCustomRetriableStrategy {
    fn handle(&self, outcome: Result<reqwest::Response, Error>) -> Option<Retryable> {
        match outcome {
              Ok(response) => // Do some custom stuff,
              Err(e) => DefaultRetriableStrategy::default().handle(Err(e))
        }
    }
}

If this pattern is clearly documented, there should be no issue in my opinion.

[tl-rodrigo-gryzinski]

That's fair. I was thinking this would be too cumbersome for one-off strategies but this is the kind of thing that you implement once for an application.

I'd include DefaultRetriableStrategy as a member of the strategy in the recommended pattern instead of instantiating a new one with each handle call though, DefaultRetriableStrategy::default could stop being free as the crate evolves. Either that or we assume we'll never add state to it and make it an unit struct: Err(e) => DefaultRetriableStrategy.handle(e), so it being a free instantiation is part of the public API

[AxelGreen]

Hello! When can we expect that functionality to be added? It can help us a lot.

[LukeMathWalker]

We can discuss merging once the issues I raised are addressed @AxelGreen 👍🏻

@tl-rodrigo-gryzinski: thinking about it again, considering that RetryableStrategy is storing function pointers we do get that initialisation is free by construction, don't we?

[conradludgate]

Hey, @Rutgerdj, Are you interested in driving this forward? I'm happy to take over if not

[Rutgerdj]

@conradludgate Sure, feel free to take over!

[conradludgate]

Have refactored this to fit more inline with how we handle tracing spans in reqwest-tracing