Skip to content

Commit

Permalink
Show when we retried requests
Browse files Browse the repository at this point in the history
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 TrueLayer/reqwest-middleware#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 TrueLayer/reqwest-middleware#159.
  • Loading branch information
konstin committed Jul 2, 2024
1 parent 8dabc29 commit 7c9fb86
Show file tree
Hide file tree
Showing 8 changed files with 81 additions and 52 deletions.
9 changes: 4 additions & 5 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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/konstin/reqwest-middleware", rev = "21ceec9a5fd2e8d6f71c3ea2999078fecbd13cbe" }
reqwest-retry = { git = "https://github.com/konstin/reqwest-middleware", rev = "21ceec9a5fd2e8d6f71c3ea2999078fecbd13cbe" }
rkyv = { version = "0.7.43", features = ["strict", "validation"] }
rmp-serde = { version = "1.1.2" }
rust-netrc = { version = "0.1.1" }
Expand Down Expand Up @@ -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/konstin/reqwest-middleware", rev = "21ceec9a5fd2e8d6f71c3ea2999078fecbd13cbe" }

[workspace.lints.rust]
unsafe_code = "warn"
Expand Down
95 changes: 62 additions & 33 deletions crates/uv-client/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 },
Expand Down Expand Up @@ -233,59 +227,91 @@ pub enum ErrorKind {

impl From<reqwest::Error> for ErrorKind {
fn from(error: reqwest::Error) -> Self {
Self::ReqwestError(BetterReqwestError::from(error))
Self::WrappedReqwestError(WrappedReqwestError::from(error))
}
}

impl From<reqwest_middleware::Error> 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::<OfflineError>() {
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);

impl BetterReqwestError {
pub struct WrappedReqwestError(reqwest_middleware::Error);

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::<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;
}
// 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<reqwest::Error> for BetterReqwestError {
impl From<reqwest::Error> for WrappedReqwestError {
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)
}
}

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?")
Expand All @@ -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()
}
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-client/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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::{
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-client/src/registry_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
8 changes: 4 additions & 4 deletions crates/uv-distribution/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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),

Expand Down Expand Up @@ -130,7 +130,7 @@ pub enum Error {

impl From<reqwest::Error> for Error {
fn from(error: reqwest::Error) -> Self {
Self::Reqwest(BetterReqwestError::from(error))
Self::Reqwest(WrappedReqwestError::from(error))
}
}

Expand All @@ -139,7 +139,7 @@ impl From<reqwest_middleware::Error> 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))
}
}
}
Expand Down
8 changes: 4 additions & 4 deletions crates/uv-toolchain/src/downloads.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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}")]
Expand Down Expand Up @@ -450,7 +450,7 @@ impl PythonDownload {

impl From<reqwest::Error> for Error {
fn from(error: reqwest::Error) -> Self {
Self::NetworkError(BetterReqwestError::from(error))
Self::NetworkError(WrappedReqwestError::from(error))
}
}

Expand All @@ -459,7 +459,7 @@ impl From<reqwest_middleware::Error> 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))
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions crates/uv/src/commands/self_update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -103,7 +103,7 @@ pub(crate) async fn self_update(printer: Printer) -> Result<ExitStatus> {
}
Err(err) => {
return Err(if let AxoupdateError::Reqwest(err) = err {
BetterReqwestError::from(err).into()
WrappedReqwestError::from(err).into()
} else {
err.into()
});
Expand Down

0 comments on commit 7c9fb86

Please sign in to comment.