From 4b193194858707795b5b64dd479f65ef22fed312 Mon Sep 17 00:00:00 2001 From: konsti Date: Tue, 2 Jul 2024 19:04:11 +0200 Subject: [PATCH] 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. --- Cargo.lock | 9 ++- Cargo.toml | 5 +- crates/uv-client/src/error.rs | 93 ++++++++++++++++--------- crates/uv-client/src/lib.rs | 2 +- crates/uv-client/src/registry_client.rs | 2 +- crates/uv-distribution/src/error.rs | 8 +-- crates/uv-toolchain/src/downloads.rs | 8 +-- crates/uv/src/commands/self_update.rs | 4 +- 8 files changed, 80 insertions(+), 51 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7218f91fa..c65a5fa33 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3118,8 +3118,7 @@ dependencies = [ [[package]] name = "reqwest-middleware" version = "0.3.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "39346a33ddfe6be00cbc17a34ce996818b97b230b87229f10114693becca1268" +source = "git+https://github.com/astral-sh/reqwest-middleware?rev=21ceec9a5fd2e8d6f71c3ea2999078fecbd13cbe#21ceec9a5fd2e8d6f71c3ea2999078fecbd13cbe" dependencies = [ "anyhow", "async-trait", @@ -3132,9 +3131,8 @@ dependencies = [ [[package]] name = "reqwest-retry" -version = "0.6.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cf2a94ba69ceb30c42079a137e2793d6d0f62e581a24c06cd4e9bb32e973c7da" +version = "0.7.0" +source = "git+https://github.com/astral-sh/reqwest-middleware?rev=21ceec9a5fd2e8d6f71c3ea2999078fecbd13cbe#21ceec9a5fd2e8d6f71c3ea2999078fecbd13cbe" dependencies = [ "anyhow", "async-trait", @@ -3147,6 +3145,7 @@ dependencies = [ "reqwest", "reqwest-middleware", "retry-policies", + "thiserror", "tokio", "tracing", "wasm-timer", diff --git a/Cargo.toml b/Cargo.toml index bd8843040..fe3dc1233 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -113,8 +113,8 @@ rayon = { version = "1.8.0" } reflink-copy = { version = "0.1.15" } 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-middleware = { version = "0.3.0" } -reqwest-retry = { version = "0.6.0" } +reqwest-middleware = { git = "https://github.com/astral-sh/reqwest-middleware", rev = "21ceec9a5fd2e8d6f71c3ea2999078fecbd13cbe" } +reqwest-retry = { git = "https://github.com/astral-sh/reqwest-middleware", rev = "21ceec9a5fd2e8d6f71c3ea2999078fecbd13cbe" } rkyv = { version = "0.7.43", features = ["strict", "validation"] } rmp-serde = { version = "1.1.2" } rust-netrc = { version = "0.1.1" } @@ -159,6 +159,7 @@ ignored = ["flate2"] # For pyproject-toml pep440_rs = { path = "crates/pep440-rs" } pep508_rs = { path = "crates/pep508-rs" } +reqwest-middleware = { git = "https://github.com/astral-sh/reqwest-middleware", rev = "21ceec9a5fd2e8d6f71c3ea2999078fecbd13cbe" } [workspace.lints.rust] unsafe_code = "warn" diff --git a/crates/uv-client/src/error.rs b/crates/uv-client/src/error.rs index 202f6ec7c..070c6fd3b 100644 --- a/crates/uv-client/src/error.rs +++ b/crates/uv-client/src/error.rs @@ -63,7 +63,7 @@ impl Error { // The server returned a "Method Not Allowed" error, indicating it doesn't support // HEAD requests, so we can't check for range requests. - ErrorKind::ReqwestError(err) => { + ErrorKind::WrappedReqwestError(err) => { if let Some(status) = err.status() { // If the server doesn't support HEAD requests, we can't check for range // requests. @@ -169,15 +169,9 @@ pub enum ErrorKind { #[error("Metadata file `{0}` was not found in {1}")] MetadataNotFound(WheelFilename, String), - /// A generic request error happened while making a request. Refer to the - /// error message for more details. + /// An error that happened while making a request or in a reqwest middleware. #[error(transparent)] - ReqwestError(#[from] BetterReqwestError), - - /// A generic request middleware error happened while making a request. - /// Refer to the error message for more details. - #[error(transparent)] - ReqwestMiddlewareError(#[from] anyhow::Error), + WrappedReqwestError(#[from] WrappedReqwestError), #[error("Received some unexpected JSON from {url}")] BadJson { source: serde_json::Error, url: Url }, @@ -233,59 +227,91 @@ pub enum ErrorKind { impl From for ErrorKind { fn from(error: reqwest::Error) -> Self { - Self::ReqwestError(BetterReqwestError::from(error)) + Self::WrappedReqwestError(WrappedReqwestError::from(error)) } } impl From for ErrorKind { - fn from(error: reqwest_middleware::Error) -> Self { - if let reqwest_middleware::Error::Middleware(ref underlying) = error { + fn from(err: reqwest_middleware::Error) -> Self { + if let reqwest_middleware::Error::Middleware(ref underlying) = err { if let Some(err) = underlying.downcast_ref::() { return Self::Offline(err.url().to_string()); } } - match error { - reqwest_middleware::Error::Middleware(err) => Self::ReqwestMiddlewareError(err), - reqwest_middleware::Error::Reqwest(err) => Self::from(err), - } + Self::WrappedReqwestError(WrappedReqwestError(err)) } } /// Handle the case with no internet by explicitly telling the user instead of showing an obscure /// 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)] -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 { - if !self.0.is_connect() { - return false; + 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::() { + Some(err) + } else if let Some(reqwest_middleware::Error::Reqwest(err)) = + err.downcast_ref::() + { + Some(err) + } else { + None + } + }), + }; + + if let Some(reqwest_err) = reqwest_err { + if !reqwest_err.is_connect() { + return false; + } + // 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 + // are opaque. + if std::error::Error::source(&reqwest_err) + .and_then(|err| err.source()) + .is_some_and(|err| err.to_string().starts_with("dns error: ")) + { + return true; + } } - // 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 - // are opaque. - std::error::Error::source(&self.0) - .and_then(|err| err.source()) - .is_some_and(|err| err.to_string().starts_with("dns error: ")) + false } } -impl From for BetterReqwestError { +impl From for WrappedReqwestError { fn from(error: reqwest::Error) -> Self { + Self(error.into()) + } +} + +impl From for WrappedReqwestError { + fn from(error: reqwest_middleware::Error) -> Self { Self(error) } } -impl Deref for BetterReqwestError { - type Target = reqwest::Error; +impl Deref for WrappedReqwestError { + type Target = reqwest_middleware::Error; fn deref(&self) -> &Self::Target { &self.0 } } -impl Display for BetterReqwestError { +impl Display for WrappedReqwestError { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { if self.is_likely_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)> { 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 { self.0.source() } diff --git a/crates/uv-client/src/lib.rs b/crates/uv-client/src/lib.rs index 8ed7c0f3b..b5b5807c3 100644 --- a/crates/uv-client/src/lib.rs +++ b/crates/uv-client/src/lib.rs @@ -1,6 +1,6 @@ pub use base_client::{BaseClient, BaseClientBuilder}; 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 linehaul::LineHaul; pub use registry_client::{ diff --git a/crates/uv-client/src/registry_client.rs b/crates/uv-client/src/registry_client.rs index d0c22b6bc..ef20831ef 100644 --- a/crates/uv-client/src/registry_client.rs +++ b/crates/uv-client/src/registry_client.rs @@ -234,7 +234,7 @@ impl RegistryClient { ErrorKind::Offline(_) => continue, // The package could not be found in the remote index. - ErrorKind::ReqwestError(err) => { + ErrorKind::WrappedReqwestError(err) => { if err.status() == Some(StatusCode::NOT_FOUND) || err.status() == Some(StatusCode::UNAUTHORIZED) || err.status() == Some(StatusCode::FORBIDDEN) diff --git a/crates/uv-distribution/src/error.rs b/crates/uv-distribution/src/error.rs index 76cecb56e..36ad15161 100644 --- a/crates/uv-distribution/src/error.rs +++ b/crates/uv-distribution/src/error.rs @@ -8,7 +8,7 @@ use crate::metadata::MetadataError; use distribution_filename::WheelFilenameError; use pep440_rs::Version; use pypi_types::HashDigest; -use uv_client::BetterReqwestError; +use uv_client::WrappedReqwestError; use uv_fs::Simplified; use uv_normalize::PackageName; @@ -31,7 +31,7 @@ pub enum Error { #[error(transparent)] Git(#[from] uv_git::GitResolverError), #[error(transparent)] - Reqwest(#[from] BetterReqwestError), + Reqwest(#[from] WrappedReqwestError), #[error(transparent)] Client(#[from] uv_client::Error), @@ -130,7 +130,7 @@ pub enum Error { impl From for Error { fn from(error: reqwest::Error) -> Self { - Self::Reqwest(BetterReqwestError::from(error)) + Self::Reqwest(WrappedReqwestError::from(error)) } } @@ -139,7 +139,7 @@ impl From for Error { match error { reqwest_middleware::Error::Middleware(error) => Self::ReqwestMiddlewareError(error), reqwest_middleware::Error::Reqwest(error) => { - Self::Reqwest(BetterReqwestError::from(error)) + Self::Reqwest(WrappedReqwestError::from(error)) } } } diff --git a/crates/uv-toolchain/src/downloads.rs b/crates/uv-toolchain/src/downloads.rs index cc25ffce5..a6ef39579 100644 --- a/crates/uv-toolchain/src/downloads.rs +++ b/crates/uv-toolchain/src/downloads.rs @@ -10,7 +10,7 @@ use crate::platform::{self, Arch, Libc, Os}; use crate::toolchain::ToolchainKey; use crate::{Interpreter, PythonVersion, ToolchainRequest, VersionRequest}; use thiserror::Error; -use uv_client::BetterReqwestError; +use uv_client::WrappedReqwestError; use futures::TryStreamExt; @@ -30,7 +30,7 @@ pub enum Error { #[error("Invalid request key, too many parts: {0}")] TooManyParts(String), #[error("Download failed")] - NetworkError(#[from] BetterReqwestError), + NetworkError(#[from] WrappedReqwestError), #[error("Download failed")] NetworkMiddlewareError(#[source] anyhow::Error), #[error("Failed to extract archive: {0}")] @@ -450,7 +450,7 @@ impl PythonDownload { impl From for Error { fn from(error: reqwest::Error) -> Self { - Self::NetworkError(BetterReqwestError::from(error)) + Self::NetworkError(WrappedReqwestError::from(error)) } } @@ -459,7 +459,7 @@ impl From for Error { match error { reqwest_middleware::Error::Middleware(error) => Self::NetworkMiddlewareError(error), reqwest_middleware::Error::Reqwest(error) => { - Self::NetworkError(BetterReqwestError::from(error)) + Self::NetworkError(WrappedReqwestError::from(error)) } } } diff --git a/crates/uv/src/commands/self_update.rs b/crates/uv/src/commands/self_update.rs index 8cea63eec..1d2af2c2a 100644 --- a/crates/uv/src/commands/self_update.rs +++ b/crates/uv/src/commands/self_update.rs @@ -4,7 +4,7 @@ use anyhow::Result; use axoupdater::{AxoUpdater, AxoupdateError}; use owo_colors::OwoColorize; use tracing::debug; -use uv_client::BetterReqwestError; +use uv_client::WrappedReqwestError; use crate::commands::ExitStatus; use crate::printer::Printer; @@ -103,7 +103,7 @@ pub(crate) async fn self_update(printer: Printer) -> Result { } Err(err) => { return Err(if let AxoupdateError::Reqwest(err) = err { - BetterReqwestError::from(err).into() + WrappedReqwestError::from(err).into() } else { err.into() });