From def249f9c18280d84f29fd96978389689fb61051 Mon Sep 17 00:00:00 2001 From: jluner Date: Tue, 30 May 2017 22:15:07 -0500 Subject: [PATCH] Fixes review comments Fix some formatting items. Changes Internal error kind to preserve the original error. Changes network retry logic to inspect full error chain for spurious errors. --- src/bin/cargo.rs | 2 +- src/cargo/lib.rs | 1 + src/cargo/ops/cargo_rustc/mod.rs | 2 +- src/cargo/sources/directory.rs | 3 +- src/cargo/sources/git/source.rs | 2 +- src/cargo/sources/git/utils.rs | 4 +- src/cargo/util/errors.rs | 13 +++--- src/cargo/util/network.rs | 74 ++++++++++++++++++++++++-------- 8 files changed, 69 insertions(+), 32 deletions(-) diff --git a/src/bin/cargo.rs b/src/bin/cargo.rs index cbb8c48c8ef..09f03ef2018 100644 --- a/src/bin/cargo.rs +++ b/src/bin/cargo.rs @@ -332,7 +332,7 @@ fn execute_external_subcommand(config: &Config, cmd: &str, args: &[String]) -> C Err(e) => e, }; - if let CargoError(CargoErrorKind::ProcessErrorKind(ref perr), ..) = err { + if let &CargoErrorKind::ProcessErrorKind(ref perr) = err.kind() { if let Some(code) = perr.exit.as_ref().and_then(|c| c.code()) { return Err(CliError::code(code)); } diff --git a/src/cargo/lib.rs b/src/cargo/lib.rs index a04836a46d1..a03662927b9 100755 --- a/src/cargo/lib.rs +++ b/src/cargo/lib.rs @@ -33,6 +33,7 @@ extern crate url; use std::io; use std::fmt; use std::error::Error; + use error_chain::ChainedError; use rustc_serialize::Decodable; use serde::ser; diff --git a/src/cargo/ops/cargo_rustc/mod.rs b/src/cargo/ops/cargo_rustc/mod.rs index f52811dbd4e..c2dd2a80656 100644 --- a/src/cargo/ops/cargo_rustc/mod.rs +++ b/src/cargo/ops/cargo_rustc/mod.rs @@ -372,7 +372,7 @@ fn rustc(cx: &mut Context, unit: &Unit, exec: Arc) -> CargoResult Source for DirectorySource<'cfg> { pkg.package_id().version()) })?; - let cksum: Checksum = serde_json::from_str(&cksum) - .chain_err(|| { + let cksum: Checksum = serde_json::from_str(&cksum).chain_err(|| { format!("failed to decode `.cargo-checksum.json` of \ {} v{}", pkg.package_id().name(), diff --git a/src/cargo/sources/git/source.rs b/src/cargo/sources/git/source.rs index 43a5605ee58..634c1814bb6 100644 --- a/src/cargo/sources/git/source.rs +++ b/src/cargo/sources/git/source.rs @@ -148,7 +148,7 @@ impl<'cfg> Source for GitSource<'cfg> { trace!("updating git source `{:?}`", self.remote); let repo = self.remote.checkout(&db_path, self.config)?; - let rev = repo.rev_for(&self.reference).map_err(CargoError::to_internal)?; + let rev = repo.rev_for(&self.reference).map_err(CargoError::into_internal)?; (repo, rev) } else { (self.remote.db_at(&db_path)?, actual_rev.unwrap()) diff --git a/src/cargo/sources/git/utils.rs b/src/cargo/sources/git/utils.rs index 35252e396b9..e715d63d112 100644 --- a/src/cargo/sources/git/utils.rs +++ b/src/cargo/sources/git/utils.rs @@ -297,7 +297,7 @@ impl<'a> GitCheckout<'a> { for mut child in repo.submodules()?.into_iter() { update_submodule(repo, &mut child, cargo_config) - .map_err(CargoError::to_internal) + .map_err(CargoError::into_internal) .chain_err(|| { format!("failed to update submodule `{}`", child.name().unwrap_or("")) @@ -532,7 +532,7 @@ fn with_authentication(url: &str, cfg: &git2::Config, mut f: F) // In the case of an authentication failure (where we tried something) then // we try to give a more helpful error message about precisely what we // tried. - res.map_err(CargoError::from).map_err(|e| e.to_internal()).chain_err(|| { + res.map_err(CargoError::from).map_err(|e| e.into_internal()).chain_err(|| { let mut msg = "failed to authenticate when downloading \ repository".to_string(); if !ssh_agent_attempts.is_empty() { diff --git a/src/cargo/util/errors.rs b/src/cargo/util/errors.rs index ef5e5e4e918..11d6dfdc6bf 100644 --- a/src/cargo/util/errors.rs +++ b/src/cargo/util/errors.rs @@ -41,9 +41,9 @@ error_chain! { } errors { - Internal(description: String){ - description(&description) - display("{}", &description) + Internal(err: Box) { + description(err.description()) + display("{}", *err) } ProcessErrorKind(proc_err: ProcessError) { description(&proc_err.desc) @@ -61,9 +61,8 @@ error_chain! { } impl CargoError { - pub fn to_internal(self) -> Self { - //This is actually bad, because it loses the cause information for foreign_link - CargoError(CargoErrorKind::Internal(self.description().to_string()), self.1) + pub fn into_internal(self) -> Self { + CargoError(CargoErrorKind::Internal(Box::new(self.0)), self.1) } fn is_human(&self) -> bool { @@ -282,5 +281,5 @@ pub fn internal(error: S) -> CargoError { } fn _internal(error: &fmt::Display) -> CargoError { - CargoErrorKind::Internal(error.to_string()).into() + CargoError::from_kind(error.to_string().into()).into_internal() } diff --git a/src/cargo/util/network.rs b/src/cargo/util/network.rs index d41e1cd2e22..50bb938c359 100644 --- a/src/cargo/util/network.rs +++ b/src/cargo/util/network.rs @@ -1,29 +1,53 @@ +use std; +use std::error::Error; + +use error_chain::ChainedError; + use util::Config; use util::errors::{CargoError, CargoErrorKind, CargoResult}; use git2; +fn maybe_spurious(err: &E) -> bool + where E: ChainedError + 'static { + //Error inspection in non-verbose mode requires inspecting the + //error kind to avoid printing Internal errors. The downcasting + //machinery requires &(Error + 'static), but the iterator (and + //underlying `cause`) return &Error. Because the borrows are + //constrained to this handling method, and because the original + //error object is constrained to be 'static, we're casting away + //the borrow's actual lifetime for purposes of downcasting and + //inspecting the error chain + unsafe fn extend_lifetime(r: &Error) -> &(Error + 'static) { + std::mem::transmute::<&Error, &Error>(r) + } -fn maybe_spurious(err: &CargoError) -> bool { - match err.kind() { - &CargoErrorKind::Git(ref git_err) => { - match git_err.class() { - git2::ErrorClass::Net | - git2::ErrorClass::Os => true, - _ => false + for e in err.iter() { + let e = unsafe { extend_lifetime(e) }; + if let Some(cargo_err) = e.downcast_ref::() { + match cargo_err.kind() { + &CargoErrorKind::Git(ref git_err) => { + match git_err.class() { + git2::ErrorClass::Net | + git2::ErrorClass::Os => return true, + _ => () + } } + &CargoErrorKind::Curl(ref curl_err) + if curl_err.is_couldnt_connect() || + curl_err.is_couldnt_resolve_proxy() || + curl_err.is_couldnt_resolve_host() || + curl_err.is_operation_timedout() || + curl_err.is_recv_error() => { + return true + } + &CargoErrorKind::HttpNot200(code, ref _url) if 500 <= code && code < 600 => { + return true + } + _ => () } - &CargoErrorKind::Curl(ref curl_err) => { - curl_err.is_couldnt_connect() || - curl_err.is_couldnt_resolve_proxy() || - curl_err.is_couldnt_resolve_host() || - curl_err.is_operation_timedout() || - curl_err.is_recv_error() - } - &CargoErrorKind::HttpNot200(code, ref _url) => { - 500 <= code && code < 600 - } - _ => false + } } + false } /// Wrapper method for network call retry logic. @@ -67,3 +91,17 @@ fn with_retry_repeats_the_call_then_works() { let result = with_retry(&config, || results.pop().unwrap()); assert_eq!(result.unwrap(), ()) } + +#[test] +fn with_retry_finds_nested_spurious_errors() { + //Error HTTP codes (5xx) are considered maybe_spurious and will prompt retry + //String error messages are not considered spurious + let error1 : CargoError = CargoErrorKind::HttpNot200(501, "Uri".to_string()).into(); + let error1 = CargoError::with_chain(error1, "A non-spurious wrapping err"); + let error2 = CargoError::from_kind(CargoErrorKind::HttpNot200(502, "Uri".to_string())); + let error2 = CargoError::with_chain(error2, "A second chained error"); + let mut results: Vec> = vec![Ok(()), Err(error1), Err(error2)]; + let config = Config::default().unwrap(); + let result = with_retry(&config, || results.pop().unwrap()); + assert_eq!(result.unwrap(), ()) +}