From 898c3f6bcfa2404f1c2b40d72dc6573e8a314917 Mon Sep 17 00:00:00 2001 From: konsti Date: Mon, 4 Mar 2024 15:47:40 +0100 Subject: [PATCH] Better offline error message (#2110) Error for `uv pip compile scripts/requirements/jupyter.in` without internet: **Before** ``` error: error sending request for url (https://pypi.org/simple/jupyter/): error trying to connect: dns error: failed to lookup address information: No such host is known. (os error 11001) Caused by: error trying to connect: dns error: failed to lookup address information: No such host is known. (os error 11001) Caused by: dns error: failed to lookup address information: No such host is known. (os error 11001) Caused by: failed to lookup address information: No such host is known. (os error 11001) ``` **After** ``` error: Could not connect, are you offline? Caused by: error sending request for url (https://pypi.org/simple/django/): error trying to connect: dns error: failed to lookup address information: Temporary failure in name resolution Caused by: error trying to connect: dns error: failed to lookup address information: Temporary failure in name resolution Caused by: dns error: failed to lookup address information: Temporary failure in name resolution Caused by: failed to lookup address information: Temporary failure in name resolution ``` On linux, it would be "Temporary failure in name resolution" instead of "No such host is known. (os error 11001)". The implementation checks for "dne error" stringly as hyper errors are opaque. The danger is that this breaks with a hyper update. We still get the complete error trace since reqwest eagerly inlines errors (https://github.com/seanmonstar/reqwest/issues/2147). No test since i wouldn't know how to simulate this in cargo test. Fixes #1971 --- crates/uv-client/Cargo.toml | 1 + crates/uv-client/src/cached_client.rs | 8 +- crates/uv-client/src/error.rs | 98 ++++++++++++++++++++----- crates/uv-client/src/flat_index.rs | 4 +- crates/uv-client/src/lib.rs | 2 +- crates/uv-client/src/registry_client.rs | 20 ++--- crates/uv-distribution/src/error.rs | 9 ++- 7 files changed, 106 insertions(+), 36 deletions(-) diff --git a/crates/uv-client/Cargo.toml b/crates/uv-client/Cargo.toml index bc1813507..8ceb69c74 100644 --- a/crates/uv-client/Cargo.toml +++ b/crates/uv-client/Cargo.toml @@ -18,6 +18,7 @@ uv-normalize = { path = "../uv-normalize" } uv-warnings = { path = "../uv-warnings" } pypi-types = { path = "../pypi-types" } +anyhow = { workspace = true } async-trait = { workspace = true } async_http_range_reader = { workspace = true } async_zip = { workspace = true, features = ["tokio"] } diff --git a/crates/uv-client/src/cached_client.rs b/crates/uv-client/src/cached_client.rs index fe5806b0b..495ca84cd 100644 --- a/crates/uv-client/src/cached_client.rs +++ b/crates/uv-client/src/cached_client.rs @@ -421,9 +421,9 @@ impl CachedClient { .execute(req) .instrument(info_span!("revalidation_request", url = url.as_str())) .await - .map_err(ErrorKind::from_middleware)? + .map_err(ErrorKind::from)? .error_for_status() - .map_err(ErrorKind::RequestError)?; + .map_err(ErrorKind::from)?; match cached .cache_policy .after_response(new_cache_policy_builder, &response) @@ -459,9 +459,9 @@ impl CachedClient { .0 .execute(req) .await - .map_err(ErrorKind::from_middleware)? + .map_err(ErrorKind::from)? .error_for_status() - .map_err(ErrorKind::RequestError)?; + .map_err(ErrorKind::from)?; let cache_policy = cache_policy_builder.build(&response); let cache_policy = if cache_policy.to_archived().is_storable() { Some(Box::new(cache_policy)) diff --git a/crates/uv-client/src/error.rs b/crates/uv-client/src/error.rs index 28ec4fefe..19961286e 100644 --- a/crates/uv-client/src/error.rs +++ b/crates/uv-client/src/error.rs @@ -1,3 +1,6 @@ +use std::fmt::{Display, Formatter}; +use std::ops::Deref; + use async_http_range_reader::AsyncHttpRangeReaderError; use async_zip::error::ZipError; use url::Url; @@ -74,17 +77,17 @@ pub enum ErrorKind { /// The metadata file was not found in the registry. #[error("File `{0}` was not found in the registry at {1}.")] - FileNotFound(String, #[source] reqwest::Error), + FileNotFound(String, #[source] BetterReqwestError), /// A generic request error happened while making a request. Refer to the /// error message for more details. #[error(transparent)] - RequestError(#[from] reqwest::Error), + ReqwestError(#[from] BetterReqwestError), /// A generic request middleware error happened while making a request. /// Refer to the error message for more details. #[error(transparent)] - RequestMiddlewareError(#[from] reqwest_middleware::Error), + ReqwestMiddlewareError(#[from] anyhow::Error), #[error("Received some unexpected JSON from {url}")] BadJson { source: serde_json::Error, url: Url }, @@ -155,20 +158,6 @@ impl ErrorKind { matches!(err.kind(), std::io::ErrorKind::NotFound) } - pub(crate) fn from_middleware(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()); - } - } - - if let reqwest_middleware::Error::Reqwest(err) = err { - return Self::RequestError(err); - } - - Self::RequestMiddlewareError(err) - } - /// Returns `true` if the error is due to the server not supporting HTTP range requests. pub(crate) fn is_http_range_requests_unsupported(&self) -> bool { match self { @@ -179,7 +168,7 @@ impl ErrorKind { // The server returned a "Method Not Allowed" error, indicating it doesn't support // HEAD requests, so we can't check for range requests. - Self::RequestError(err) => { + Self::ReqwestError(err) => { if let Some(status) = err.status() { if status == reqwest::StatusCode::METHOD_NOT_ALLOWED { return true; @@ -208,3 +197,76 @@ impl ErrorKind { false } } + +impl From for ErrorKind { + fn from(error: reqwest::Error) -> Self { + Self::ReqwestError(BetterReqwestError::from(error)) + } +} +impl From for ErrorKind { + fn from(error: reqwest_middleware::Error) -> Self { + if let reqwest_middleware::Error::Middleware(ref underlying) = error { + 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), + } + } +} + +/// Handle the case with no internet by explicitly telling the user instead of showing an obscure +/// DNS error. +#[derive(Debug)] +pub struct BetterReqwestError(reqwest::Error); + +impl BetterReqwestError { + fn is_likely_offline(&self) -> bool { + if !self.0.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. + std::error::Error::source(&self.0) + .and_then(|err| err.source()) + .is_some_and(|err| err.to_string().starts_with("dns error: ")) + } +} + +impl From for BetterReqwestError { + fn from(error: reqwest::Error) -> Self { + Self(error) + } +} + +impl Deref for BetterReqwestError { + type Target = reqwest::Error; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl Display for BetterReqwestError { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + if self.is_likely_offline() { + f.write_str("Could not connect, are you offline?") + } else { + Display::fmt(&self.0, f) + } + } +} + +impl std::error::Error for BetterReqwestError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + if self.is_likely_offline() { + Some(&self.0) + } else { + self.0.source() + } + } +} diff --git a/crates/uv-client/src/flat_index.rs b/crates/uv-client/src/flat_index.rs index 034cd8cb8..ad6cc6b0b 100644 --- a/crates/uv-client/src/flat_index.rs +++ b/crates/uv-client/src/flat_index.rs @@ -152,14 +152,14 @@ impl<'a> FlatIndexClient<'a> { .header("Accept-Encoding", "gzip") .header("Accept", "text/html") .build() - .map_err(ErrorKind::RequestError)?; + .map_err(ErrorKind::from)?; let parse_simple_response = |response: Response| { async { // Use the response URL, rather than the request URL, as the base for relative URLs. // This ensures that we handle redirects and other URL transformations correctly. let url = safe_copy_url_auth(url, response.url().clone()); - let text = response.text().await.map_err(ErrorKind::RequestError)?; + let text = response.text().await.map_err(ErrorKind::from)?; let SimpleHtml { base, files } = SimpleHtml::parse(&text, &url) .map_err(|err| Error::from_html_err(err, url.clone()))?; diff --git a/crates/uv-client/src/lib.rs b/crates/uv-client/src/lib.rs index 1002f9d3f..82fdb858e 100644 --- a/crates/uv-client/src/lib.rs +++ b/crates/uv-client/src/lib.rs @@ -1,5 +1,5 @@ pub use cached_client::{CacheControl, CachedClient, CachedClientError, DataWithCachePolicy}; -pub use error::{Error, ErrorKind}; +pub use error::{BetterReqwestError, Error, ErrorKind}; pub use flat_index::{FlatDistributions, FlatIndex, FlatIndexClient, FlatIndexError}; pub use registry_client::{ Connectivity, RegistryClient, RegistryClientBuilder, SimpleMetadata, SimpleMetadatum, diff --git a/crates/uv-client/src/registry_client.rs b/crates/uv-client/src/registry_client.rs index be511b8e8..134257ccc 100644 --- a/crates/uv-client/src/registry_client.rs +++ b/crates/uv-client/src/registry_client.rs @@ -196,13 +196,13 @@ impl RegistryClient { Ok(metadata) => Ok((index.clone(), metadata)), Err(CachedClientError::Client(err)) => match err.into_kind() { ErrorKind::Offline(_) => continue, - ErrorKind::RequestError(err) => { + ErrorKind::ReqwestError(err) => { if err.status() == Some(StatusCode::NOT_FOUND) || err.status() == Some(StatusCode::FORBIDDEN) { continue; } - Err(ErrorKind::RequestError(err).into()) + Err(ErrorKind::from(err).into()) } other => Err(other.into()), }, @@ -259,7 +259,7 @@ impl RegistryClient { .header("Accept-Encoding", "gzip") .header("Accept", MediaType::accepts()) .build() - .map_err(ErrorKind::RequestError)?; + .map_err(ErrorKind::from)?; let parse_simple_response = |response: Response| { async { // Use the response URL, rather than the request URL, as the base for relative URLs. @@ -283,14 +283,14 @@ impl RegistryClient { let unarchived = match media_type { MediaType::Json => { - let bytes = response.bytes().await.map_err(ErrorKind::RequestError)?; + let bytes = response.bytes().await.map_err(ErrorKind::from)?; let data: SimpleJson = serde_json::from_slice(bytes.as_ref()) .map_err(|err| Error::from_json_err(err, url.clone()))?; SimpleMetadata::from_files(data.files, package_name, &url) } MediaType::Html => { - let text = response.text().await.map_err(ErrorKind::RequestError)?; + let text = response.text().await.map_err(ErrorKind::from)?; let SimpleHtml { base, files } = SimpleHtml::parse(&text, &url) .map_err(|err| Error::from_html_err(err, url.clone()))?; let base = safe_copy_url_auth(&url, base.into_url()); @@ -403,7 +403,7 @@ impl RegistryClient { }; let response_callback = |response: Response| async { - let bytes = response.bytes().await.map_err(ErrorKind::RequestError)?; + let bytes = response.bytes().await.map_err(ErrorKind::from)?; info_span!("parse_metadata21") .in_scope(|| Metadata21::parse(bytes.as_ref())) @@ -420,7 +420,7 @@ impl RegistryClient { .uncached() .get(url.clone()) .build() - .map_err(ErrorKind::RequestError)?; + .map_err(ErrorKind::from)?; Ok(self .client .get_serde(req, &cache_entry, cache_control, response_callback) @@ -465,7 +465,7 @@ impl RegistryClient { http::HeaderValue::from_static("identity"), ) .build() - .map_err(ErrorKind::RequestError)?; + .map_err(ErrorKind::from)?; // Copy authorization headers from the HEAD request to subsequent requests let mut headers = HeaderMap::default(); @@ -536,9 +536,9 @@ impl RegistryClient { .get(url.to_string()) .send() .await - .map_err(ErrorKind::RequestMiddlewareError)? + .map_err(ErrorKind::from)? .error_for_status() - .map_err(ErrorKind::RequestError)? + .map_err(ErrorKind::from)? .bytes_stream() .map_err(|err| std::io::Error::new(std::io::ErrorKind::Other, err)) .into_async_read(), diff --git a/crates/uv-distribution/src/error.rs b/crates/uv-distribution/src/error.rs index 156d35eab..8c50b1b09 100644 --- a/crates/uv-distribution/src/error.rs +++ b/crates/uv-distribution/src/error.rs @@ -2,6 +2,7 @@ use tokio::task::JoinError; use zip::result::ZipError; use distribution_filename::WheelFilenameError; +use uv_client::BetterReqwestError; use uv_normalize::PackageName; #[derive(Debug, thiserror::Error)] @@ -19,7 +20,7 @@ pub enum Error { #[error("Git operation failed")] Git(#[source] anyhow::Error), #[error(transparent)] - Request(#[from] reqwest::Error), + Reqwest(#[from] BetterReqwestError), #[error(transparent)] Client(#[from] uv_client::Error), @@ -60,3 +61,9 @@ pub enum Error { #[error("The task executor is broken, did some other task panic?")] Join(#[from] JoinError), } + +impl From for Error { + fn from(error: reqwest::Error) -> Self { + Self::Reqwest(BetterReqwestError::from(error)) + } +}