Merge pull request #2121 from kinnison/kinnison/retry-downloads

Kinnison/retry downloads

Author: kinnison

CONTRIBUTING.md

@@ -140,6 +140,13 @@ than 1, it is clamped to 1 at minimum.
 This is not meant for use by users, but can be suggested in diagnosing an issue
 should one arise with the backtrack limits.
 
+### `RUSTUP_MAX_RETRIES`
+
+When downloading a file, rustup will retry the download a number of times. The
+default is 3 times, but if this variable is set to a valid usize then it is the
+max retry count. A value of `0` means no retries, thus the default of `3` will
+mean a download is tried a total of four times before failing out.
+
 ### `RUSTUP_BACKTRACE`
 
 By default while running tests, we unset some environment variables that will

src/dist/manifestation.rs

@@ -12,6 +12,8 @@ use crate::dist::prefix::InstallPrefix;
 use crate::dist::temp;
 use crate::errors::*;
 use crate::utils::utils;
+use retry::delay::NoDelay;
+use retry::{retry, OperationResult};
 use std::path::Path;
 
 pub const DIST_MANIFEST: &str = "multirust-channel-manifest.toml";
@@ -151,6 +153,13 @@ impl Manifestation {
         let mut things_to_install: Vec<(Component, Format, File)> = Vec::new();
         let mut things_downloaded: Vec<String> = Vec::new();
         let components = update.components_urls_and_hashes(new_manifest)?;
+
+        const DEFAULT_MAX_RETRIES: usize = 3;
+        let max_retries: usize = std::env::var("RUSTUP_MAX_RETRIES")
+            .ok()
+            .and_then(|s| s.parse().ok())
+            .unwrap_or(DEFAULT_MAX_RETRIES);
+
         for (component, format, url, hash) in components {
             notify_handler(Notification::DownloadingComponent(
                 &component.short_name(new_manifest),
@@ -165,9 +174,21 @@ impl Manifestation {
 
             let url_url = utils::parse_url(&url)?;
 
-            let downloaded_file = download_cfg
-                .download(&url_url, &hash)
-                .chain_err(|| ErrorKind::ComponentDownloadFailed(component.name(new_manifest)))?;
+            let downloaded_file = retry(NoDelay.take(max_retries), || {
+                match download_cfg.download(&url_url, &hash) {
+                    Ok(f) => OperationResult::Ok(f),
+                    Err(e) => match e.kind() {
+                        // If there was a broken partial file, try again
+                        ErrorKind::DownloadingFile { .. } | ErrorKind::BrokenPartialFile => {
+                            notify_handler(Notification::RetryingDownload(&url));
+                            OperationResult::Retry(e)
+                        }
+
+                        _ => OperationResult::Err(e),
+                    },
+                }
+            })
+            .chain_err(|| ErrorKind::ComponentDownloadFailed(component.name(new_manifest)))?;
 
             things_downloaded.push(hash);
 

src/dist/notifications.rs

@@ -37,6 +37,7 @@ pub enum Notification<'a> {
     ComponentUnavailable(&'a str, Option<&'a TargetTriple>),
     StrayHash(&'a Path),
     SignatureInvalid(&'a str),
+    RetryingDownload(&'a str),
 }
 
 impl<'a> From<crate::utils::Notification<'a>> for Notification<'a> {
@@ -72,6 +73,7 @@ impl<'a> Notification<'a> {
             | RollingBack
             | DownloadingManifest(_)
             | SkippingNightlyMissingComponent(_)
+            | RetryingDownload(_)
             | DownloadedManifest(_, _) => NotificationLevel::Info,
             CantReadUpdateHash(_)
             | ExtensionNotInstalled(_)
@@ -181,6 +183,7 @@ impl<'a> Display for Notification<'a> {
                 write!(f, "Force-skipping unavailable component '{}'", component)
             }
             SignatureInvalid(url) => write!(f, "Signature verification failed for '{}'", url),
+            RetryingDownload(url) => write!(f, "Retrying download for '{}'", url),
         }
     }
 }

src/utils/utils.rs

@@ -172,6 +172,9 @@ pub fn download_file_with_resume(
         Ok(_) => Ok(()),
         Err(e) => {
             let is_client_error = match e.kind() {
+                // Specifically treat the bad partial range error as not our
+                // fault in case it was something odd which happened.
+                ErrorKind::Download(DEK::HttpStatus(416)) => false,
                 ErrorKind::Download(DEK::HttpStatus(400..=499)) => true,
                 ErrorKind::Download(DEK::FileNotFound) => true,
                 _ => false,
@@ -243,11 +246,12 @@ fn download_file_(
         (Backend::Reqwest, Notification::UsingReqwest)
     };
     notify_handler(notification);
-    download_to_path_with_backend(backend, url, path, resume_from_partial, Some(callback))?;
+    let res =
+        download_to_path_with_backend(backend, url, path, resume_from_partial, Some(callback));
 
     notify_handler(Notification::DownloadFinished);
 
-    Ok(())
+    res.map_err(|e| e.into())
 }
 
 pub fn parse_url(url: &str) -> Result<Url> {

tests/dist.rs

@@ -2018,30 +2018,13 @@ fn handle_corrupt_partial_downloads() {
         )
         .unwrap();
 
-        let err = update_from_dist(
-            url,
-            toolchain,
-            prefix,
-            &[],
-            &[],
-            download_cfg,
-            temp_cfg,
-            false,
-        )
-        .unwrap_err();
-
-        match err.kind() {
-            ErrorKind::ComponentDownloadFailed(_) => (),
-            e => panic!("Unexpected error: {:?}", e),
-        }
-
         update_from_dist(
             url,
             toolchain,
             prefix,
             &[],
             &[],
-            &download_cfg,
+            download_cfg,
             temp_cfg,
             false,
         )