search

TrueLayer / reqwest-middleware

Wrapper around reqwest to allow for client middleware chains.
Apache License 2.0
257 stars 78 forks source link

Add retries to error message #159

Open konstin opened 4 months ago

konstin commented 4 months ago

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. https://github.com/astral-sh/uv/issues/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.

aochagavia commented 2 months ago

@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 commented 2 months ago

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