Auto merge of #2396 - sbeckeriv:network-retry-1602, r=alexcrichton

Network retry issue 1602

Dearest Reviewer,

This branch resolves #1602 which relates to retrying network
issues automatically. There is a new utility helper for retrying
any network call.

There is a new config called net.retry value in the .cargo.config
file. The default value is 2. The documentation has also been
updated to reflect the new value.

Thanks
Becker

Cargo.toml

@@ -21,6 +21,7 @@ advapi32-sys = "0.1"
 crates-io = { path = "src/crates-io", version = "0.2" }
 crossbeam = "0.2"
 curl = "0.2"
+curl-sys = "0.1"
 docopt = "0.6"
 env_logger = "0.3"
 filetime = "0.1"

src/cargo/lib.rs

@@ -6,6 +6,7 @@
 extern crate crates_io as registry;
 extern crate crossbeam;
 extern crate curl;
+extern crate curl_sys;
 extern crate docopt;
 extern crate filetime;
 extern crate flate2;

src/cargo/sources/git/source.rs

@@ -162,7 +162,8 @@ impl<'cfg> Source for GitSource<'cfg> {
                 format!("git repository `{}`", self.remote.url())));
 
             trace!("updating git source `{:?}`", self.remote);
-            let repo = try!(self.remote.checkout(&db_path));
+
+            let repo = try!(self.remote.checkout(&db_path, &self.config));
             let rev = try!(repo.rev_for(&self.reference));
             (repo, rev)
         } else {
@@ -172,7 +173,7 @@ impl<'cfg> Source for GitSource<'cfg> {
         // Copy the database to the checkout location. After this we could drop
         // the lock on the database as we no longer needed it, but we leave it
         // in scope so the destructors here won't tamper with too much.
-        try!(repo.copy_to(actual_rev.clone(), &checkout_path));
+        try!(repo.copy_to(actual_rev.clone(), &checkout_path, &self.config));
 
         let source_id = self.source_id.with_precise(Some(actual_rev.to_string()));
         let path_source = PathSource::new_recursive(&checkout_path,

src/cargo/sources/git/utils.rs

@@ -8,7 +8,7 @@ use url::Url;
 use git2::{self, ObjectType};
 
 use core::GitReference;
-use util::{CargoResult, ChainError, human, ToUrl, internal};
+use util::{CargoResult, ChainError, human, ToUrl, internal, Config, network};
 
 #[derive(PartialEq, Clone, Debug)]
 pub struct GitRevision(git2::Oid);
@@ -109,16 +109,16 @@ impl GitRemote {
         db.rev_for(reference)
     }
 
-    pub fn checkout(&self, into: &Path) -> CargoResult<GitDatabase> {
+    pub fn checkout(&self, into: &Path, cargo_config: &Config) -> CargoResult<GitDatabase> {
         let repo = match git2::Repository::open(into) {
             Ok(repo) => {
-                try!(self.fetch_into(&repo).chain_error(|| {
+                try!(self.fetch_into(&repo, &cargo_config).chain_error(|| {
                     human(format!("failed to fetch into {}", into.display()))
                 }));
                 repo
             }
             Err(..) => {
-                try!(self.clone_into(into).chain_error(|| {
+                try!(self.clone_into(into, &cargo_config).chain_error(|| {
                     human(format!("failed to clone into: {}", into.display()))
                 }))
             }
@@ -140,21 +140,21 @@ impl GitRemote {
         })
     }
 
-    fn fetch_into(&self, dst: &git2::Repository) -> CargoResult<()> {
+    fn fetch_into(&self, dst: &git2::Repository, cargo_config: &Config) -> CargoResult<()> {
         // Create a local anonymous remote in the repository to fetch the url
         let url = self.url.to_string();
         let refspec = "refs/heads/*:refs/heads/*";
-        fetch(dst, &url, refspec)
+        fetch(dst, &url, refspec, &cargo_config)
     }
 
-    fn clone_into(&self, dst: &Path) -> CargoResult<git2::Repository> {
+    fn clone_into(&self, dst: &Path, cargo_config: &Config) -> CargoResult<git2::Repository> {
         let url = self.url.to_string();
         if fs::metadata(&dst).is_ok() {
             try!(fs::remove_dir_all(dst));
         }
         try!(fs::create_dir_all(dst));
         let repo = try!(git2::Repository::init_bare(dst));
-        try!(fetch(&repo, &url, "refs/heads/*:refs/heads/*"));
+        try!(fetch(&repo, &url, "refs/heads/*:refs/heads/*", &cargo_config));
         Ok(repo)
     }
 }
@@ -164,21 +164,21 @@ impl GitDatabase {
         &self.path
     }
 
-    pub fn copy_to(&self, rev: GitRevision, dest: &Path)
+    pub fn copy_to(&self, rev: GitRevision, dest: &Path, cargo_config: &Config)
                    -> CargoResult<GitCheckout> {
         let checkout = match git2::Repository::open(dest) {
             Ok(repo) => {
                 let checkout = GitCheckout::new(dest, self, rev, repo);
                 if !checkout.is_fresh() {
-                    try!(checkout.fetch());
+                    try!(checkout.fetch(&cargo_config));
                     try!(checkout.reset());
                     assert!(checkout.is_fresh());
                 }
                 checkout
             }
             Err(..) => try!(GitCheckout::clone_into(dest, self, rev)),
         };
-        try!(checkout.update_submodules().chain_error(|| {
+        try!(checkout.update_submodules(&cargo_config).chain_error(|| {
             internal("failed to update submodules")
         }));
         Ok(checkout)
@@ -276,12 +276,12 @@ impl<'a> GitCheckout<'a> {
         }
     }
 
-    fn fetch(&self) -> CargoResult<()> {
+    fn fetch(&self, cargo_config: &Config) -> CargoResult<()> {
         info!("fetch {}", self.repo.path().display());
         let url = try!(self.database.path.to_url().map_err(human));
         let url = url.to_string();
         let refspec = "refs/heads/*:refs/heads/*";
-        try!(fetch(&self.repo, &url, refspec));
+        try!(fetch(&self.repo, &url, refspec, &cargo_config));
         Ok(())
     }
 
@@ -303,10 +303,10 @@ impl<'a> GitCheckout<'a> {
         Ok(())
     }
 
-    fn update_submodules(&self) -> CargoResult<()> {
-        return update_submodules(&self.repo);
+    fn update_submodules(&self, cargo_config: &Config) -> CargoResult<()> {
+        return update_submodules(&self.repo, &cargo_config);
 
-        fn update_submodules(repo: &git2::Repository) -> CargoResult<()> {
+        fn update_submodules(repo: &git2::Repository, cargo_config: &Config) -> CargoResult<()> {
             info!("update submodules for: {:?}", repo.workdir().unwrap());
 
             for mut child in try!(repo.submodules()).into_iter() {
@@ -346,14 +346,14 @@ impl<'a> GitCheckout<'a> {
 
                 // Fetch data from origin and reset to the head commit
                 let refspec = "refs/heads/*:refs/heads/*";
-                try!(fetch(&repo, url, refspec).chain_error(|| {
+                try!(fetch(&repo, url, refspec, &cargo_config).chain_error(|| {
                     internal(format!("failed to fetch submodule `{}` from {}",
                                      child.name().unwrap_or(""), url))
                 }));
 
                 let obj = try!(repo.find_object(head, None));
                 try!(repo.reset(&obj, git2::ResetType::Hard, None));
-                try!(update_submodules(&repo));
+                try!(update_submodules(&repo, &cargo_config));
             }
             Ok(())
         }
@@ -389,7 +389,7 @@ impl<'a> GitCheckout<'a> {
 /// attempted and we don't try the same ones again.
 fn with_authentication<T, F>(url: &str, cfg: &git2::Config, mut f: F)
                              -> CargoResult<T>
-    where F: FnMut(&mut git2::Credentials) -> Result<T, git2::Error>
+    where F: FnMut(&mut git2::Credentials) -> CargoResult<T>
 {
     let mut cred_helper = git2::CredentialHelper::new(url);
     cred_helper.config(cfg);
@@ -555,7 +555,7 @@ fn with_authentication<T, F>(url: &str, cfg: &git2::Config, mut f: F)
 }
 
 pub fn fetch(repo: &git2::Repository, url: &str,
-             refspec: &str) -> CargoResult<()> {
+             refspec: &str, cargo_config: &Config) -> CargoResult<()> {
     // Create a local anonymous remote in the repository to fetch the url
 
     with_authentication(url, &try!(repo.config()), |f| {
@@ -565,7 +565,10 @@ pub fn fetch(repo: &git2::Repository, url: &str,
         let mut opts = git2::FetchOptions::new();
         opts.remote_callbacks(cb)
             .download_tags(git2::AutotagOption::All);
-        try!(remote.fetch(&[refspec], Some(&mut opts), None));
+
+        try!(network::with_retry(cargo_config, ||{
+            remote.fetch(&[refspec], Some(&mut opts), None)
+        }));
         Ok(())
     })
 }

src/cargo/sources/registry.rs

@@ -483,7 +483,8 @@ impl<'cfg> RegistrySource<'cfg> {
         // git fetch origin
         let url = self.source_id.url().to_string();
         let refspec = "refs/heads/*:refs/remotes/origin/*";
-        try!(git::fetch(&repo, &url, refspec).chain_error(|| {
+
+        try!(git::fetch(&repo, &url, refspec, &self.config).chain_error(|| {
             internal(format!("failed to fetch `{}`", url))
         }));
 

src/cargo/util/config.rs

@@ -265,6 +265,21 @@ impl Config {
         }
     }
 
+    pub fn net_retry(&self) -> CargoResult<i64> {
+        match try!(self.get_i64("net.retry")) {
+            Some(v) => {
+                let value = v.val;
+                if value < 0 {
+                    bail!("net.retry must be positive, but found {} in {}",
+                      v.val, v.definition)
+                } else {
+                    Ok(value)
+                }
+            }
+            None => Ok(2),
+        }
+    }
+
     pub fn expected<T>(&self, ty: &str, key: &str, val: CV) -> CargoResult<T> {
         val.expected(ty).map_err(|e| {
             human(format!("invalid configuration for key `{}`\n{}", key, e))

src/cargo/util/errors.rs

@@ -8,6 +8,7 @@ use std::str;
 use std::string;
 
 use curl;
+use curl_sys;
 use git2;
 use rustc_serialize::json;
 use semver;
@@ -285,6 +286,36 @@ impl From<Box<CargoError>> for CliError {
     }
 }
 
+// =============================================================================
+// NetworkError trait
+
+pub trait NetworkError: CargoError {
+    fn maybe_spurious(&self) -> bool;
+}
+
+impl NetworkError for git2::Error {
+    fn maybe_spurious(&self) -> bool {
+        match self.class() {
+            git2::ErrorClass::Net |
+            git2::ErrorClass::Os => true,
+            _ => false
+        }
+    }
+}
+impl NetworkError for curl::ErrCode {
+    fn maybe_spurious(&self) -> bool {
+        match self.code()  {
+            curl_sys::CURLcode::CURLE_COULDNT_CONNECT |
+            curl_sys::CURLcode::CURLE_COULDNT_RESOLVE_PROXY |
+            curl_sys::CURLcode::CURLE_COULDNT_RESOLVE_HOST |
+            curl_sys::CURLcode::CURLE_OPERATION_TIMEDOUT |
+            curl_sys::CURLcode::CURLE_RECV_ERROR
+            => true,
+            _ => false
+        }
+    }
+}
+
 // =============================================================================
 // various impls
 

src/cargo/util/mod.rs

@@ -32,6 +32,7 @@ pub mod to_url;
 pub mod toml;
 pub mod lev_distance;
 pub mod job;
+pub mod network;
 mod cfg;
 mod dependency_queue;
 mod rustc;

src/cargo/util/network.rs

@@ -0,0 +1,86 @@
+use util::{CargoResult, Config, errors};
+
+/// Wrapper method for network call retry logic.
+///
+/// Retry counts provided by Config object 'net.retry'. Config shell outputs
+/// a warning on per retry.
+///
+/// Closure must return a CargoResult.
+///
+/// Example:
+/// use util::network;
+/// cargo_result = network.with_retry(&config, || something.download());
+pub fn with_retry<T, E, F>(config: &Config, mut callback: F) -> CargoResult<T>
+    where F: FnMut() -> Result<T, E>,
+          E: errors::NetworkError
+{
+    let mut remaining = try!(config.net_retry());
+    loop {
+        match callback() {
+            Ok(ret) => return Ok(ret),
+            Err(ref e) if e.maybe_spurious() && remaining > 0 => {
+                let msg = format!("spurious network error ({} tries \
+                          remaining): {}", remaining, e);
+                try!(config.shell().warn(msg));
+                remaining -= 1;
+            }
+            Err(e) => return Err(Box::new(e)),
+        }
+    }
+}
+#[test]
+fn with_retry_repeats_the_call_then_works() {
+
+    use std::error::Error;
+    use util::human;
+    use std::fmt;
+
+    #[derive(Debug)]
+    struct NetworkRetryError {
+        error: Box<errors::CargoError>,
+    }
+
+    impl Error for NetworkRetryError {
+        fn description(&self) -> &str {
+            self.error.description()
+        }
+        fn cause(&self) -> Option<&Error> {
+            self.error.cause()
+        }
+    }
+
+    impl NetworkRetryError {
+        fn new(error: &str) -> NetworkRetryError {
+            let error = human(error.to_string());
+            NetworkRetryError {
+                error: error,
+            }
+        }
+    }
+
+    impl fmt::Display for NetworkRetryError {
+        fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+            fmt::Display::fmt(&self.error, f)
+        }
+    }
+
+    impl errors::CargoError for NetworkRetryError {
+        fn is_human(&self) -> bool {
+            false
+        }
+    }
+
+    impl errors::NetworkError for NetworkRetryError {
+        fn maybe_spurious(&self) -> bool {
+            true
+        }
+    }
+
+    let error1 = NetworkRetryError::new("one");
+    let error2 = NetworkRetryError::new("two");
+    let mut results: Vec<Result<(), NetworkRetryError>> = vec![Ok(()),
+    Err(error1), Err(error2)];
+    let config = Config::default().unwrap();
+    let result = with_retry(&config, || results.pop().unwrap());
+    assert_eq!(result.unwrap(), ())
+}

src/doc/config.md

@@ -89,6 +89,10 @@ rustflags = ["..", ".."]  # custom flags to pass to all compiler invocations
 [term]
 verbose = false        # whether cargo provides verbose output
 color = 'auto'         # whether cargo colorizes output
+
+# Network configuration
+[net]
+retry = 2 # number of times a network call will automatically retried
 ```
 
 # Environment Variables

tests/support/mod.rs

@@ -662,6 +662,7 @@ fn substitute_macros(input: &str) -> String {
         ("[RUNNING]",     "     Running"),
         ("[COMPILING]",   "   Compiling"),
         ("[ERROR]",       "error:"),
+        ("[WARNING]",     "warning:"),
         ("[DOCUMENTING]", " Documenting"),
         ("[FRESH]",       "       Fresh"),
         ("[UPDATING]",    "    Updating"),