Merge pull request #1889 from Glamhoth/partial-fix

Removing .partial that causes repeated failure to download a component

Author: kinnison

download/src/lib.rs

@@ -42,6 +42,7 @@ pub fn download_to_path_with_backend(
     callback: Option<&dyn Fn(Event<'_>) -> Result<()>>,
 ) -> Result<()> {
     use std::cell::RefCell;
+    use std::fs::remove_file;
     use std::fs::OpenOptions;
     use std::io::{Read, Seek, SeekFrom, Write};
 
@@ -114,7 +115,10 @@ pub fn download_to_path_with_backend(
         Ok(())
     }()
     .map_err(|e| {
-        // TODO is there any point clearing up here? What kind of errors will leave us with an unusable partial?
+        // TODO: We currently clear up the cached download on any error, should we restrict it to a subset?
+        remove_file(path)
+            .chain_err(|| "cleaning up cached downloads")
+            .unwrap();
         e
     })
 }

src/dist/download.rs

@@ -68,26 +68,39 @@ impl<'a> DownloadCfg<'a> {
                 + ".partial",
         );
 
+        let partial_file_existed = partial_file_path.exists();
+
         let mut hasher = Sha256::new();
 
-        utils::download_file_with_resume(
+        if let Err(e) = utils::download_file_with_resume(
             &url,
             &partial_file_path,
             Some(&mut hasher),
             true,
             &|n| (self.notify_handler)(n.into()),
-        )?;
+        ) {
+            if partial_file_existed {
+                return Err(e).chain_err(|| ErrorKind::BrokenPartialFile);
+            } else {
+                return Err(e);
+            }
+        };
 
         let actual_hash = format!("{:x}", hasher.result());
 
         if hash != actual_hash {
             // Incorrect hash
-            Err(ErrorKind::ChecksumFailed {
-                url: url.to_string(),
-                expected: hash.to_string(),
-                calculated: actual_hash,
+            if partial_file_existed {
+                self.clean(&[hash.to_string() + &".partial".to_string()])?;
+                Err(ErrorKind::BrokenPartialFile.into())
+            } else {
+                Err(ErrorKind::ChecksumFailed {
+                    url: url.to_string(),
+                    expected: hash.to_string(),
+                    calculated: actual_hash,
+                }
+                .into())
             }
-            .into())
         } else {
             (self.notify_handler)(Notification::ChecksumValid(&url.to_string()));
 

src/errors.rs

@@ -362,6 +362,9 @@ error_chain! {
             description("invalid PGP key"),
             display("unable to read the PGP key '{}'", v.display())
         }
+        BrokenPartialFile {
+            description("partially downloaded file may have been damaged and was removed, please try again")
+        }
     }
 }
 

tests/dist.rs

@@ -25,6 +25,8 @@ use std::str::FromStr;
 use std::sync::Arc;
 use url::Url;
 
+const SHA256_HASH_LEN: usize = 64;
+
 // Creates a mock dist server populated with some test data
 pub fn create_mock_dist_server(
     path: &Path,
@@ -1984,3 +1986,68 @@ fn checks_files_hashes_before_reuse() {
         assert!(noticed_bad_checksum.get());
     })
 }
+
+#[test]
+fn handle_corrupt_partial_downloads() {
+    setup(None, false, &|url,
+                         toolchain,
+                         prefix,
+                         download_cfg,
+                         temp_cfg| {
+        // write a corrupt partial out
+        let path = url.to_file_path().unwrap();
+        let target_hash = utils::read_file(
+            "target hash",
+            &path.join("dist/2016-02-02/rustc-nightly-x86_64-apple-darwin.tar.gz.sha256"),
+        )
+        .unwrap()[..SHA256_HASH_LEN]
+            .to_owned();
+
+        utils::ensure_dir_exists(
+            "download dir",
+            &download_cfg.download_dir,
+            &|_: Notification<'_>| {},
+        )
+        .unwrap();
+        let partial_path = download_cfg
+            .download_dir
+            .join(format!("{}.partial", target_hash));
+        utils_raw::write_file(
+            &partial_path,
+            "file will be resumed from here and not match hash",
+        )
+        .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,
+            temp_cfg,
+            false,
+        )
+        .unwrap();
+
+        assert!(utils::path_exists(&prefix.path().join("bin/rustc")));
+        assert!(utils::path_exists(&prefix.path().join("lib/libstd.rlib")));
+    });
+}