Add retries to error message

[konstin]

As a user or developer, it is hard to tell whether a request failed randomly and was not retried, or whether it failed even with retries (e.g. astral-sh/uv#3514). This PR adds the number of retries to the error chain if there were any retries. Example error chain:

error: Request failed after 3 retries
  Caused by: error sending request for url (https://pypi.org/simple/tqdm/)
  Caused by: client error (Connect)
  Caused by: dns error: failed to lookup address information: Name or service not known
  Caused by: failed to lookup address information: Name or service not known

I've also changed request_middleware::Error transparent to avoid being to verbose.

[cbeck88]

suggestion:

using an enum here is going to make it harder for a user to match on this error now, because they have to handle an extra branch that is still just a reqwest_middleware::Error

it seems completely unnecessary, why don't you just make this a struct that has a retry count and an error?
the only place where RetryError::Error was constructed, we actually do have a retry count that we could supply.

[cbeck88]

for example, suppose I'm a user, and i want to do further error handling and call is_body on the underlying Reqwest::Error. https://docs.rs/reqwest/latest/reqwest/struct.Error.html#method.is_body
With this enum that requires a lot more boilerplate code

[konstin]

We need the enum so we can insert the error message about the number of retries when we did them, while not showing an irrelevant "Failed after 0 retries" otherwise.

[cbeck88]

that doesn't seem like such a big problem. maybe you could customize Display so that it doesn't show when the number is 0? the enum is going to create boilerplate at any call-site that wants to further inspect the error

[konstin]

We can't skip the display on 0 because it ToString has to return a string even if it's an empty string, the only way i know of how to skip a step in the error trace is error(transparent).

[cbeck88]

We can't skip the display on 0 because it ToString has to return a string even if it's an empty string

i think maybe you misunderstood me, you could do something like this

pub struct Error {
   err: reqwest::Error,
   retries: u32,
}

then you could implement Display like

impl Display for Error {
    fn fmt(&self, f: ...) -> ... {
       if self.retries == 0 {
          write!(f, "{}", self.err)
       } else {
          write!(f, "Failed after {} retries: {}", self.retries, self.err)
       }
    }
}

or similar

[cbeck88]

this looks wrong, on this code path, this is not a middleware error, it's a request error.

a good example of a middleware error is when the middleware cannot do something it needs to do to function, like at like 146 above.

the result variable is the result of actually making a request and it failing or succeeding somehow, so these errors are request errors, not middleware errors.

[cbeck88]

to make this work you need to change ReqwestMiddleware::Error so that the Reqwest variant contains your retry data

[konstin]

I can't change reqwest::Error since it's part of the reqwest crate and not of request-middleware crate, so we need to use the dynamic error variant here.

[cbeck88]

you can change reqwest_middleware::Error , in fact you already did in this PR. that's what i'm suggesting

[konstin]

That would require changing reqwest_middleware::Error so that it has a variant specific to retry errors, this would break the isolation between the general retry stack and the specific middleware for retrying.

[cbeck88]

yeah i see now, that's annoying

[cbeck88]

this is also not a middleware error

[konstin]

It's the most common pattern in rust to have one error type per crate (or sometimes per top level module, like serde::de::Error and serde::ser::Error). Crates higher up in the dependency tree then have an enum where most variants are wrappers around the errors of crates they depend on. In reqwest-middleware, we want to allow a dynamic middleware stack, so neither do we know the list of error type statically, nor can we declare it at compile time, so we have to use a dyn Error such as anyhow provides. The current request_middleware::Error is insufficient when adding context as we do here, it doesn't properly model that the actual reqwest::Error instance may be lower in the stack. I don't really want to change that type to just a anyhow::Error wrapper with a method that finds you a reqwest::Error in the chain, if any, to avoid too much churn.

[cbeck88]

the change you are making here, where normal reqwest::Error from the crate, is classified as reqwest_middleware::Error::middleware instead of reqwest_middleware::Error::request, is going to break every single existing user of this crate who attempts to actually inspect and handle these errors. because today, all those users match on reqwest_middleware::Error and 99.9% of the time it's a request error that they can further inspect.

[cbeck88]

it almost completely defeats the point of having request_middleware::Error::reqwest, which after this change will serve to only confuse users

[cbeck88]

imo, these changelog entries are not sufficient, you should indicate a semver breaking change

[konstin]

The current interpretation of the error enum assumes two cases:

• There was an internal error in the middleware
• There was a reqwest::Error we forward
  But there exists a third class of errors, those where we do have a reqwest::Error, but also additional context or a middle error with a reqwest::Error behind it. The current enum represent this kind of errors, and there exists no simple rust pattern for matching on them.

[konstin]

Good point, i've bumped reqwest-retry to 0.6.0, let's see what the maintainers say about reqwest-middleware.

[aochagavia]

@cbeck88 @konstin is there anything preventing this PR from being merged? I'm using uv as a dependency in a project and had to patch reqwest-middleware in my Cargo.toml, and I know there are other projects that are refraining from update to the latest uv libraries to avoid this issue (e.g. pixi).

Let me know if I can help in any way here ;)

[cbeck88]

@cbeck88 @konstin is there anything preventing this PR from being merged? I'm using uv as a dependency in a project and had to patch reqwest-middleware in my Cargo.toml, and I know there are other projects that are refraining from update to the latest uv libraries to avoid this issue (e.g. pixi).

Let me know if I can help in any way here ;)

At a minimum, the cargo toml has to be fixed, because 0.6.0 was already released

We haven't yet heard from maintainers whether they want to go in this direction. Currently the number of retries is reported by logging and the log level is configurable.

If they do, imo more thought should be given to exactly what the structure of the error is, because existing code that needs to access the underlying reqwest error will have to downcast, and any changes to that structure in the future will be breaking changes. Maybe better to have private fields and pub fn getters.

If they don't want the patch, there's probably still a path forward for uv to get the number of retries without forking this crate, for example by injecting a custom strategy object that wraps the default one and contains a counter, or adding another middleware layer I guess.

[eopb]

@konstin first of all, I'm very sorry this took so long before getting reviewed by a maintainer. Thank you for putting in the effort to continually rebase this PR. It's awesome to see reqwest-retry being used in uv.

@cbeck88 thank you for providing such detailed reviews. These were very helpful.

I don't see anything major preventing this from being merged so I'm going to go ahead. There's been a fair amount of discussion so feel welcome to open a PR or issue if anyone disagrees. Another breaking change to reqwest-retry wouldn't be a huge deal.