Skip to content

Removing .partial that causes repeated failure to download a component #1889

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 3, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion download/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand Down Expand Up @@ -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
})
}
Expand Down
27 changes: 20 additions & 7 deletions src/dist/download.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()));

Expand Down
3 changes: 3 additions & 0 deletions src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
}
}

Expand Down
67 changes: 67 additions & 0 deletions tests/dist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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")));
});
}