Show when we retried requests (#4725)

In #3514 and #2755, users had intermittent network errors, but it was
not always clear whether we had already retried these requests or not.
Building upon https://github.com/TrueLayer/reqwest-middleware/pull/159,
this PR adds the number of retries to the error message, so we can see
at first glance where we're missing retries and where we might need to
change retry settings.

Example error trace:

```
Could not connect, are you offline?
  Caused by: Request failed after 3 retries
  Caused by: error sending request for url (https://pypi.org/simple/uv/)
  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
```

This code is ugly since i'm missing a better pattern for attaching
context to reqwest middleware errors in
https://github.com/TrueLayer/reqwest-middleware/pull/159.
This commit is contained in:
konsti 2024-07-02 19:04:11 +02:00 committed by GitHub
parent 12e12d066d
commit 4b19319485
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
8 changed files with 80 additions and 51 deletions

9
Cargo.lock generated
View file

@ -3118,8 +3118,7 @@ dependencies = [
[[package]] [[package]]
name = "reqwest-middleware" name = "reqwest-middleware"
version = "0.3.2" version = "0.3.2"
source = "registry+https://github.com/rust-lang/crates.io-index" source = "git+https://github.com/astral-sh/reqwest-middleware?rev=21ceec9a5fd2e8d6f71c3ea2999078fecbd13cbe#21ceec9a5fd2e8d6f71c3ea2999078fecbd13cbe"
checksum = "39346a33ddfe6be00cbc17a34ce996818b97b230b87229f10114693becca1268"
dependencies = [ dependencies = [
"anyhow", "anyhow",
"async-trait", "async-trait",
@ -3132,9 +3131,8 @@ dependencies = [
[[package]] [[package]]
name = "reqwest-retry" name = "reqwest-retry"
version = "0.6.0" version = "0.7.0"
source = "registry+https://github.com/rust-lang/crates.io-index" source = "git+https://github.com/astral-sh/reqwest-middleware?rev=21ceec9a5fd2e8d6f71c3ea2999078fecbd13cbe#21ceec9a5fd2e8d6f71c3ea2999078fecbd13cbe"
checksum = "cf2a94ba69ceb30c42079a137e2793d6d0f62e581a24c06cd4e9bb32e973c7da"
dependencies = [ dependencies = [
"anyhow", "anyhow",
"async-trait", "async-trait",
@ -3147,6 +3145,7 @@ dependencies = [
"reqwest", "reqwest",
"reqwest-middleware", "reqwest-middleware",
"retry-policies", "retry-policies",
"thiserror",
"tokio", "tokio",
"tracing", "tracing",
"wasm-timer", "wasm-timer",

View file

@ -113,8 +113,8 @@ rayon = { version = "1.8.0" }
reflink-copy = { version = "0.1.15" } reflink-copy = { version = "0.1.15" }
regex = { version = "1.10.2" } regex = { version = "1.10.2" }
reqwest = { version = "0.12.3", default-features = false, features = ["json", "gzip", "brotli", "stream", "rustls-tls", "rustls-tls-native-roots"] } reqwest = { version = "0.12.3", default-features = false, features = ["json", "gzip", "brotli", "stream", "rustls-tls", "rustls-tls-native-roots"] }
reqwest-middleware = { version = "0.3.0" } reqwest-middleware = { git = "https://github.com/astral-sh/reqwest-middleware", rev = "21ceec9a5fd2e8d6f71c3ea2999078fecbd13cbe" }
reqwest-retry = { version = "0.6.0" } reqwest-retry = { git = "https://github.com/astral-sh/reqwest-middleware", rev = "21ceec9a5fd2e8d6f71c3ea2999078fecbd13cbe" }
rkyv = { version = "0.7.43", features = ["strict", "validation"] } rkyv = { version = "0.7.43", features = ["strict", "validation"] }
rmp-serde = { version = "1.1.2" } rmp-serde = { version = "1.1.2" }
rust-netrc = { version = "0.1.1" } rust-netrc = { version = "0.1.1" }
@ -159,6 +159,7 @@ ignored = ["flate2"]
# For pyproject-toml # For pyproject-toml
pep440_rs = { path = "crates/pep440-rs" } pep440_rs = { path = "crates/pep440-rs" }
pep508_rs = { path = "crates/pep508-rs" } pep508_rs = { path = "crates/pep508-rs" }
reqwest-middleware = { git = "https://github.com/astral-sh/reqwest-middleware", rev = "21ceec9a5fd2e8d6f71c3ea2999078fecbd13cbe" }
[workspace.lints.rust] [workspace.lints.rust]
unsafe_code = "warn" unsafe_code = "warn"

View file

@ -63,7 +63,7 @@ impl Error {
// The server returned a "Method Not Allowed" error, indicating it doesn't support // The server returned a "Method Not Allowed" error, indicating it doesn't support
// HEAD requests, so we can't check for range requests. // HEAD requests, so we can't check for range requests.
ErrorKind::ReqwestError(err) => { ErrorKind::WrappedReqwestError(err) => {
if let Some(status) = err.status() { if let Some(status) = err.status() {
// If the server doesn't support HEAD requests, we can't check for range // If the server doesn't support HEAD requests, we can't check for range
// requests. // requests.
@ -169,15 +169,9 @@ pub enum ErrorKind {
#[error("Metadata file `{0}` was not found in {1}")] #[error("Metadata file `{0}` was not found in {1}")]
MetadataNotFound(WheelFilename, String), MetadataNotFound(WheelFilename, String),
/// A generic request error happened while making a request. Refer to the /// An error that happened while making a request or in a reqwest middleware.
/// error message for more details.
#[error(transparent)] #[error(transparent)]
ReqwestError(#[from] BetterReqwestError), WrappedReqwestError(#[from] WrappedReqwestError),
/// A generic request middleware error happened while making a request.
/// Refer to the error message for more details.
#[error(transparent)]
ReqwestMiddlewareError(#[from] anyhow::Error),
#[error("Received some unexpected JSON from {url}")] #[error("Received some unexpected JSON from {url}")]
BadJson { source: serde_json::Error, url: Url }, BadJson { source: serde_json::Error, url: Url },
@ -233,59 +227,91 @@ pub enum ErrorKind {
impl From<reqwest::Error> for ErrorKind { impl From<reqwest::Error> for ErrorKind {
fn from(error: reqwest::Error) -> Self { fn from(error: reqwest::Error) -> Self {
Self::ReqwestError(BetterReqwestError::from(error)) Self::WrappedReqwestError(WrappedReqwestError::from(error))
} }
} }
impl From<reqwest_middleware::Error> for ErrorKind { impl From<reqwest_middleware::Error> for ErrorKind {
fn from(error: reqwest_middleware::Error) -> Self { fn from(err: reqwest_middleware::Error) -> Self {
if let reqwest_middleware::Error::Middleware(ref underlying) = error { if let reqwest_middleware::Error::Middleware(ref underlying) = err {
if let Some(err) = underlying.downcast_ref::<OfflineError>() { if let Some(err) = underlying.downcast_ref::<OfflineError>() {
return Self::Offline(err.url().to_string()); return Self::Offline(err.url().to_string());
} }
} }
match error { Self::WrappedReqwestError(WrappedReqwestError(err))
reqwest_middleware::Error::Middleware(err) => Self::ReqwestMiddlewareError(err),
reqwest_middleware::Error::Reqwest(err) => Self::from(err),
}
} }
} }
/// Handle the case with no internet by explicitly telling the user instead of showing an obscure /// Handle the case with no internet by explicitly telling the user instead of showing an obscure
/// DNS error. /// DNS error.
///
/// Wraps a [`reqwest_middleware::Error`] instead of an [`reqwest::Error`] since the actual reqwest
/// error may be below some context in the [`anyhow::Error`].
#[derive(Debug)] #[derive(Debug)]
pub struct BetterReqwestError(reqwest::Error); pub struct WrappedReqwestError(reqwest_middleware::Error);
impl BetterReqwestError { impl WrappedReqwestError {
/// Check if the error chain contains a reqwest error that looks like this:
/// * error sending request for url (...)
/// * client error (Connect)
/// * dns error: failed to lookup address information: Name or service not known
/// * failed to lookup address information: Name or service not known
fn is_likely_offline(&self) -> bool { fn is_likely_offline(&self) -> bool {
if !self.0.is_connect() { let reqwest_err = match &self.0 {
reqwest_middleware::Error::Reqwest(err) => Some(err),
reqwest_middleware::Error::Middleware(err) => err.chain().find_map(|err| {
if let Some(err) = err.downcast_ref::<reqwest::Error>() {
Some(err)
} else if let Some(reqwest_middleware::Error::Reqwest(err)) =
err.downcast_ref::<reqwest_middleware::Error>()
{
Some(err)
} else {
None
}
}),
};
if let Some(reqwest_err) = reqwest_err {
if !reqwest_err.is_connect() {
return false; return false;
} }
// Self is "error sending request for url", the first source is "error trying to connect", // Self is "error sending request for url", the first source is "error trying to connect",
// the second source is "dns error". We have to check for the string because hyper errors // the second source is "dns error". We have to check for the string because hyper errors
// are opaque. // are opaque.
std::error::Error::source(&self.0) if std::error::Error::source(&reqwest_err)
.and_then(|err| err.source()) .and_then(|err| err.source())
.is_some_and(|err| err.to_string().starts_with("dns error: ")) .is_some_and(|err| err.to_string().starts_with("dns error: "))
{
return true;
}
}
false
} }
} }
impl From<reqwest::Error> for BetterReqwestError { impl From<reqwest::Error> for WrappedReqwestError {
fn from(error: reqwest::Error) -> Self { fn from(error: reqwest::Error) -> Self {
Self(error.into())
}
}
impl From<reqwest_middleware::Error> for WrappedReqwestError {
fn from(error: reqwest_middleware::Error) -> Self {
Self(error) Self(error)
} }
} }
impl Deref for BetterReqwestError { impl Deref for WrappedReqwestError {
type Target = reqwest::Error; type Target = reqwest_middleware::Error;
fn deref(&self) -> &Self::Target { fn deref(&self) -> &Self::Target {
&self.0 &self.0
} }
} }
impl Display for BetterReqwestError { impl Display for WrappedReqwestError {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
if self.is_likely_offline() { if self.is_likely_offline() {
f.write_str("Could not connect, are you offline?") f.write_str("Could not connect, are you offline?")
@ -295,10 +321,13 @@ impl Display for BetterReqwestError {
} }
} }
impl std::error::Error for BetterReqwestError { impl std::error::Error for WrappedReqwestError {
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
if self.is_likely_offline() { if self.is_likely_offline() {
Some(&self.0) match &self.0 {
reqwest_middleware::Error::Middleware(err) => Some(err.as_ref()),
reqwest_middleware::Error::Reqwest(err) => Some(err),
}
} else { } else {
self.0.source() self.0.source()
} }

View file

@ -1,6 +1,6 @@
pub use base_client::{BaseClient, BaseClientBuilder}; pub use base_client::{BaseClient, BaseClientBuilder};
pub use cached_client::{CacheControl, CachedClient, CachedClientError, DataWithCachePolicy}; pub use cached_client::{CacheControl, CachedClient, CachedClientError, DataWithCachePolicy};
pub use error::{BetterReqwestError, Error, ErrorKind}; pub use error::{Error, ErrorKind, WrappedReqwestError};
pub use flat_index::{FlatIndexClient, FlatIndexEntries, FlatIndexError}; pub use flat_index::{FlatIndexClient, FlatIndexEntries, FlatIndexError};
pub use linehaul::LineHaul; pub use linehaul::LineHaul;
pub use registry_client::{ pub use registry_client::{

View file

@ -234,7 +234,7 @@ impl RegistryClient {
ErrorKind::Offline(_) => continue, ErrorKind::Offline(_) => continue,
// The package could not be found in the remote index. // The package could not be found in the remote index.
ErrorKind::ReqwestError(err) => { ErrorKind::WrappedReqwestError(err) => {
if err.status() == Some(StatusCode::NOT_FOUND) if err.status() == Some(StatusCode::NOT_FOUND)
|| err.status() == Some(StatusCode::UNAUTHORIZED) || err.status() == Some(StatusCode::UNAUTHORIZED)
|| err.status() == Some(StatusCode::FORBIDDEN) || err.status() == Some(StatusCode::FORBIDDEN)

View file

@ -8,7 +8,7 @@ use crate::metadata::MetadataError;
use distribution_filename::WheelFilenameError; use distribution_filename::WheelFilenameError;
use pep440_rs::Version; use pep440_rs::Version;
use pypi_types::HashDigest; use pypi_types::HashDigest;
use uv_client::BetterReqwestError; use uv_client::WrappedReqwestError;
use uv_fs::Simplified; use uv_fs::Simplified;
use uv_normalize::PackageName; use uv_normalize::PackageName;
@ -31,7 +31,7 @@ pub enum Error {
#[error(transparent)] #[error(transparent)]
Git(#[from] uv_git::GitResolverError), Git(#[from] uv_git::GitResolverError),
#[error(transparent)] #[error(transparent)]
Reqwest(#[from] BetterReqwestError), Reqwest(#[from] WrappedReqwestError),
#[error(transparent)] #[error(transparent)]
Client(#[from] uv_client::Error), Client(#[from] uv_client::Error),
@ -130,7 +130,7 @@ pub enum Error {
impl From<reqwest::Error> for Error { impl From<reqwest::Error> for Error {
fn from(error: reqwest::Error) -> Self { fn from(error: reqwest::Error) -> Self {
Self::Reqwest(BetterReqwestError::from(error)) Self::Reqwest(WrappedReqwestError::from(error))
} }
} }
@ -139,7 +139,7 @@ impl From<reqwest_middleware::Error> for Error {
match error { match error {
reqwest_middleware::Error::Middleware(error) => Self::ReqwestMiddlewareError(error), reqwest_middleware::Error::Middleware(error) => Self::ReqwestMiddlewareError(error),
reqwest_middleware::Error::Reqwest(error) => { reqwest_middleware::Error::Reqwest(error) => {
Self::Reqwest(BetterReqwestError::from(error)) Self::Reqwest(WrappedReqwestError::from(error))
} }
} }
} }

View file

@ -10,7 +10,7 @@ use crate::platform::{self, Arch, Libc, Os};
use crate::toolchain::ToolchainKey; use crate::toolchain::ToolchainKey;
use crate::{Interpreter, PythonVersion, ToolchainRequest, VersionRequest}; use crate::{Interpreter, PythonVersion, ToolchainRequest, VersionRequest};
use thiserror::Error; use thiserror::Error;
use uv_client::BetterReqwestError; use uv_client::WrappedReqwestError;
use futures::TryStreamExt; use futures::TryStreamExt;
@ -30,7 +30,7 @@ pub enum Error {
#[error("Invalid request key, too many parts: {0}")] #[error("Invalid request key, too many parts: {0}")]
TooManyParts(String), TooManyParts(String),
#[error("Download failed")] #[error("Download failed")]
NetworkError(#[from] BetterReqwestError), NetworkError(#[from] WrappedReqwestError),
#[error("Download failed")] #[error("Download failed")]
NetworkMiddlewareError(#[source] anyhow::Error), NetworkMiddlewareError(#[source] anyhow::Error),
#[error("Failed to extract archive: {0}")] #[error("Failed to extract archive: {0}")]
@ -450,7 +450,7 @@ impl PythonDownload {
impl From<reqwest::Error> for Error { impl From<reqwest::Error> for Error {
fn from(error: reqwest::Error) -> Self { fn from(error: reqwest::Error) -> Self {
Self::NetworkError(BetterReqwestError::from(error)) Self::NetworkError(WrappedReqwestError::from(error))
} }
} }
@ -459,7 +459,7 @@ impl From<reqwest_middleware::Error> for Error {
match error { match error {
reqwest_middleware::Error::Middleware(error) => Self::NetworkMiddlewareError(error), reqwest_middleware::Error::Middleware(error) => Self::NetworkMiddlewareError(error),
reqwest_middleware::Error::Reqwest(error) => { reqwest_middleware::Error::Reqwest(error) => {
Self::NetworkError(BetterReqwestError::from(error)) Self::NetworkError(WrappedReqwestError::from(error))
} }
} }
} }

View file

@ -4,7 +4,7 @@ use anyhow::Result;
use axoupdater::{AxoUpdater, AxoupdateError}; use axoupdater::{AxoUpdater, AxoupdateError};
use owo_colors::OwoColorize; use owo_colors::OwoColorize;
use tracing::debug; use tracing::debug;
use uv_client::BetterReqwestError; use uv_client::WrappedReqwestError;
use crate::commands::ExitStatus; use crate::commands::ExitStatus;
use crate::printer::Printer; use crate::printer::Printer;
@ -103,7 +103,7 @@ pub(crate) async fn self_update(printer: Printer) -> Result<ExitStatus> {
} }
Err(err) => { Err(err) => {
return Err(if let AxoupdateError::Reqwest(err) = err { return Err(if let AxoupdateError::Reqwest(err) = err {
BetterReqwestError::from(err).into() WrappedReqwestError::from(err).into()
} else { } else {
err.into() err.into()
}); });