Implement retry::always for a retry policy that applies to all domains

[oscartbeaumont]

This is useful because in my case I want to have a global client for sharing the connection pool but I don't know the domains ahead of time for the retry policy as our application allows dynamic URLs. I know that I want all requests on this specific reqwest::Client to use the retry policy.

This functionality could theoretically be implemented in userspace in the future but currently the Scope trait is sealed making that impossible so this would need to exist in reqwest from my current understanding.

This is an opposite of the existing reqwest::never method.

[seanmonstar]

Thanks for the PR!

This is an addition I'm quite hesitant about.

The idea of "always" is a little hard to describe, because it's not really always. If it were, that would mean that every response is retried (even if "successful"), and you'd loop forever. In contrast, it's much easier to say what "never" means.

When I was originally working on reqwest's retry module, my goals were to ease adding HTTP retries safely. It was already possible to do retries without reqwest's help, but when I would look at where people had done so, they often forget certain protections. So, one could do retries externally just in a loop. I'd want to open it up so advanced users can grab pieces from tower-http and hopefully still be safe but do more of what they need. And in reqwest::retry, I want that module to nudge and guide and almost shove users towards "this way is safer".

So, after saying all that, I think a possible next step would be a slightly more flexible way of defining scopes, with a healthy dose of "please be careful in here".

[oscartbeaumont]

I may misunderstand but from my understanding using always() is kinda equivalent to a conceptual from_hosts("*") where * is all domains. With an assigned clarify_fn my understanding is that would "contain" what is retired to only what you expect? I suppose the issue your outlining is that when using always() the default is truely always and that could lead to problems without a clarify_fn? I am not sure I fully grasp why from_host is okay but a version that supported all hosts would not?

I would be curious to hear more about how you see a solution here working as I would be potentially be interested in contributing if I can get a better understanding! I do fully understand that this may require more thought on your end so all good if you can't outline it exactly!

I think one clear solution would be exposing more of the retry traits (Scope, etc) but these were sealed intentionally so I suspect this is more of a good idea in theory and not good idea in practice unless your really able to stand behind the current design which obviously for something like this takes time. I wonder if we could have a builder method which takes in a closure and allows an end-user to implement whatever they want. This could internally be wrapped into a ScopeFn. This could give the benefits of exposing it while also keeping it mostly sealed although it still requires locking in some-form of the design for the arguments to that closure. This method would probally end up being redundant if Scope is ever exposed but it could also be soft-deprecated at that point so that might not be the end of the world but is also feels a little bit messy to be doing intentionally?

I suspect this probably isn't the place for discussion on this but for my specific usecase I think defining the retry policy on the request would be a lot more flexible than on the client. This is leading me to believe maybe I should do a userspace retry implementation but i'm also aiming to reduce code we have to maintain and as discussed above retry code can be easy to get wrong. Previously we defined a new reqwest::Client for every API call due to the dynamic hostname for it, but currently I am aiming to reuse the reqwest::Client across our application to help with connection pooling and cross-request rate limits but i'm noticing this is coming at a massive cost to flexibility when defining the retry policy hence my PR here. Would be curious if you have thoughts here? Can we do something to share the connection pool while having distinct configurations across multiple reqwest::Client's or something else i'm missing?

Thanks for your time! I really appreciate the open source work you do!

[seanmonstar]

I may misunderstand but from my understanding using always() is kinda equivalent to a conceptual from_hosts("*") where * is all domains.

Yes, that is a correct interpretation too. What I worry about is what does this mean to a user when they read it:

Client::builder()
    .retries(reqwest::retry::always())
    .etc()

When contrast with never(), it reads to me that all requests are retried. Unfortunately, it might seem slot into the user's expectation that reqwest understands what is considered a retryable response, but it wouldn't. And it'd be missing a classifier. Where as never() works since it doesn't need a classifier.

(It's possible that the API design could have been adjusted slightly to include a couple generics such that if you said never(), that's a ready builder, but scoped(host) or all_hosts() etc would have a typestate set of WantsClassifier. We could possibly change that in a breaking release, if that type enforcement would be better.)