Skip to content

Commit

Permalink
fix: display reqwest::Error source
Browse files Browse the repository at this point in the history
Since reqwest 0.12.1 (seanmonstar/reqwest#2199),
fmt::Display for reqwest::Error no longer includes the source.

The whole reason for WrappedReqwestError was to avoid displaying the source
twice. But since the above change, the source isn't displayed at all.

After this change, dfx will once again display the error source(s) for
request::Error, but on separate lines ("Caused by:") like other error sources.
  • Loading branch information
ericswanson-dfinity committed Jul 15, 2024
1 parent 349d547 commit 29d902c
Show file tree
Hide file tree
Showing 8 changed files with 28 additions and 48 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@

# UNRELEASED

# fix: display error cause of some http-related errors

Some commands that download http resources, for example `dfx extension install`, will
once again display any error cause.

# 0.22.0

### asset uploads: retry some HTTP errors returned by the replica
Expand Down
7 changes: 3 additions & 4 deletions src/dfx-core/src/error/extension.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
#![allow(dead_code)]
use crate::error::reqwest::WrappedReqwestError;
use crate::error::structured_file::StructuredFileError;
use thiserror::Error;

Expand Down Expand Up @@ -84,7 +83,7 @@ pub enum NewExtensionManagerError {
#[derive(Error, Debug)]
pub enum DownloadAndInstallExtensionToTempdirError {
#[error(transparent)]
ExtensionDownloadFailed(WrappedReqwestError),
ExtensionDownloadFailed(reqwest::Error),

#[error("Cannot get extensions directory")]
EnsureExtensionDirExistsFailed(#[source] crate::error::fs::FsError),
Expand Down Expand Up @@ -141,10 +140,10 @@ pub enum GetDependenciesError {
ParseUrl(#[from] url::ParseError),

#[error(transparent)]
Get(WrappedReqwestError),
Get(reqwest::Error),

#[error(transparent)]
ParseJson(WrappedReqwestError),
ParseJson(reqwest::Error),
}

#[derive(Error, Debug)]
Expand Down
36 changes: 12 additions & 24 deletions src/dfx-core/src/error/reqwest.rs
Original file line number Diff line number Diff line change
@@ -1,28 +1,16 @@
use crate::http::retryable::Retryable;
use reqwest::StatusCode;
use thiserror::Error;

// reqwest::Error's fmt::Display appends the error descriptions of all sources.
// For this reason, it is not marked as #[source] here, so that we don't
// display the error descriptions of all sources repeatedly.
#[derive(Error, Debug)]
#[error("{}", .0)]
pub struct WrappedReqwestError(pub reqwest::Error);

impl Retryable for WrappedReqwestError {
fn is_retryable(&self) -> bool {
let err = &self.0;
err.is_timeout()
|| err.is_connect()
|| matches!(
err.status(),
Some(
StatusCode::INTERNAL_SERVER_ERROR
| StatusCode::BAD_GATEWAY
| StatusCode::SERVICE_UNAVAILABLE
| StatusCode::GATEWAY_TIMEOUT
| StatusCode::TOO_MANY_REQUESTS
)
pub fn is_retryable(err: &reqwest::Error) -> bool {
err.is_timeout()
|| err.is_connect()
|| matches!(
err.status(),
Some(
StatusCode::INTERNAL_SERVER_ERROR
| StatusCode::BAD_GATEWAY
| StatusCode::SERVICE_UNAVAILABLE
| StatusCode::GATEWAY_TIMEOUT
| StatusCode::TOO_MANY_REQUESTS
)
}
)
}
10 changes: 4 additions & 6 deletions src/dfx-core/src/extension/manager/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ use crate::error::extension::{
GetExtensionArchiveNameError, GetExtensionDownloadUrlError, GetHighestCompatibleVersionError,
InstallExtensionError,
};
use crate::error::reqwest::WrappedReqwestError;
use crate::extension::{
manager::ExtensionManager, manifest::ExtensionDependencies, url::ExtensionJsonUrl,
};
Expand Down Expand Up @@ -96,11 +95,10 @@ impl ExtensionManager {
.await
.map_err(DownloadAndInstallExtensionToTempdirError::ExtensionDownloadFailed)?;

let bytes = response.bytes().await.map_err(|e| {
DownloadAndInstallExtensionToTempdirError::ExtensionDownloadFailed(WrappedReqwestError(
e,
))
})?;
let bytes = response
.bytes()
.await
.map_err(DownloadAndInstallExtensionToTempdirError::ExtensionDownloadFailed)?;

crate::fs::composite::ensure_dir_exists(&self.dir)
.map_err(DownloadAndInstallExtensionToTempdirError::EnsureExtensionDirExistsFailed)?;
Expand Down
5 changes: 1 addition & 4 deletions src/dfx-core/src/extension/manifest/dependencies.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use crate::error::extension::{DfxOnlySupportedDependency, GetDependenciesError};
use crate::error::reqwest::WrappedReqwestError;
use crate::extension::url::ExtensionJsonUrl;
use crate::http::get::get_with_retries;
use crate::json::structure::VersionReqWithJsonSchema;
Expand Down Expand Up @@ -36,9 +35,7 @@ impl ExtensionDependencies {
.await
.map_err(GetDependenciesError::Get)?;

resp.json()
.await
.map_err(|e| GetDependenciesError::ParseJson(WrappedReqwestError(e)))
resp.json().await.map_err(GetDependenciesError::ParseJson)
}

pub fn find_highest_compatible_version(
Expand Down
9 changes: 3 additions & 6 deletions src/dfx-core/src/http/get.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
use crate::error::reqwest::WrappedReqwestError;
use crate::http::retryable::Retryable;
use backoff::exponential::ExponentialBackoff;
use backoff::future::retry;
use backoff::SystemClock;
Expand All @@ -9,15 +7,14 @@ use url::Url;
pub async fn get_with_retries(
url: Url,
retry_policy: ExponentialBackoff<SystemClock>,
) -> Result<Response, WrappedReqwestError> {
) -> Result<Response, reqwest::Error> {
let operation = || async {
let response = reqwest::get(url.clone())
.await
.and_then(|resp| resp.error_for_status())
.map_err(WrappedReqwestError);
.and_then(|resp| resp.error_for_status());
match response {
Ok(doc) => Ok(doc),
Err(e) if e.is_retryable() => Err(backoff::Error::transient(e)),
Err(e) if crate::error::reqwest::is_retryable(&e) => Err(backoff::Error::transient(e)),
Err(e) => Err(backoff::Error::permanent(e)),
}
};
Expand Down
1 change: 0 additions & 1 deletion src/dfx-core/src/http/mod.rs
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
pub mod get;
pub mod retryable;
3 changes: 0 additions & 3 deletions src/dfx-core/src/http/retryable.rs

This file was deleted.

0 comments on commit 29d902c

Please sign in to comment.