diff --git a/Cargo.lock b/Cargo.lock index e3c102e4e9..c1e45bc0f0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -182,6 +182,7 @@ dependencies = [ "native-tls 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", "openssl-probe 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", "rustls 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)", + "tempdir 0.3.4 (registry+https://github.com/rust-lang/crates.io-index)", "url 1.1.1 (registry+https://github.com/rust-lang/crates.io-index)", ] diff --git a/ci/run.sh b/ci/run.sh index 465551f4df..516d9317c7 100644 --- a/ci/run.sh +++ b/ci/run.sh @@ -10,6 +10,7 @@ cargo -vV cargo build --release --target $TARGET if [ -z "$SKIP_TESTS" ]; then + cargo test --release -p download --target $TARGET cargo test --release -p rustup-dist --target $TARGET cargo test --release --target $TARGET fi diff --git a/src/download/Cargo.toml b/src/download/Cargo.toml index 264c764a22..dbccf1c596 100644 --- a/src/download/Cargo.toml +++ b/src/download/Cargo.toml @@ -21,6 +21,9 @@ curl = { version = "0.4", optional = true } lazy_static = { version = "0.2", optional = true } native-tls = { version = "0.1", optional = true } +[dev-dependencies] +tempdir = "0.3.4" + [dependencies.hyper] version = "0.9.8" default-features = false diff --git a/src/download/src/errors.rs b/src/download/src/errors.rs index 45b5d0cc27..6dfada40cb 100644 --- a/src/download/src/errors.rs +++ b/src/download/src/errors.rs @@ -1,7 +1,11 @@ + + error_chain! { links { } - foreign_links { } + foreign_links { + Io(::std::io::Error); + } errors { HttpStatus(e: u32) { diff --git a/src/download/src/lib.rs b/src/download/src/lib.rs index 821ce5fc17..01855f4277 100644 --- a/src/download/src/lib.rs +++ b/src/download/src/lib.rs @@ -21,6 +21,7 @@ pub enum Backend { Curl, Hyper, Rustls } #[derive(Debug, Copy, Clone)] pub enum Event<'a> { + ResumingPartialDownload, /// Received the Content-Length of the to-be downloaded data. DownloadContentLengthReceived(u64), /// Received some data. @@ -33,47 +34,89 @@ const BACKENDS: &'static [Backend] = &[ Backend::Rustls ]; -pub fn download(url: &Url, - callback: &Fn(Event) -> Result<()>) - -> Result<()> { - for &backend in BACKENDS { - match download_with_backend(backend, url, callback) { - Err(Error(ErrorKind::BackendUnavailable(_), _)) => (), - Err(e) => return Err(e), - Ok(()) => return Ok(()), - } - } - - Err("no working backends".into()) -} -pub fn download_with_backend(backend: Backend, +fn download_with_backend(backend: Backend, url: &Url, + resume_from: u64, callback: &Fn(Event) -> Result<()>) -> Result<()> { match backend { - Backend::Curl => curl::download(url, callback), + Backend::Curl => curl::download(url, resume_from, callback), Backend::Hyper => hyper::download(url, callback), Backend::Rustls => rustls::download(url, callback), } } +fn supports_partial_download(backend: &Backend) -> bool { + match backend { + &Backend::Curl => true, + _ => false + } +} + pub fn download_to_path_with_backend( backend: Backend, url: &Url, path: &Path, + resume_from_partial: bool, callback: Option<&Fn(Event) -> Result<()>>) -> Result<()> { use std::cell::RefCell; - use std::fs::{self, File}; - use std::io::Write; + use std::fs::{OpenOptions}; + use std::io::{Read, Write, Seek, SeekFrom}; || -> Result<()> { - let file = RefCell::new(try!(File::create(&path).chain_err( - || "error creating file for download"))); + let (file, resume_from) = if resume_from_partial && supports_partial_download(&backend) { + let possible_partial = OpenOptions::new() + .read(true) + .open(&path); + + let downloaded_so_far = if let Ok(mut partial) = possible_partial { + if let Some(cb) = callback { + try!(cb(Event::ResumingPartialDownload)); + + let mut buf = vec![0; 32768]; + let mut downloaded_so_far = 0; + loop { + let n = try!(partial.read(&mut buf)); + downloaded_so_far += n as u64; + if n == 0 { + break; + } + try!(cb(Event::DownloadDataReceived(&buf[..n]))); + } + + downloaded_so_far + } else { + let file_info = try!(partial.metadata()); + file_info.len() + } + } else { + 0 + }; - try!(download_with_backend(backend, url, &|event| { + let mut possible_partial = + try!(OpenOptions::new() + .write(true) + .create(true) + .open(&path) + .chain_err(|| "error opening file for download")); + + try!(possible_partial.seek(SeekFrom::End(0))); + + (possible_partial, downloaded_so_far) + } else { + (try!(OpenOptions::new() + .write(true) + .create(true) + .open(&path) + .chain_err(|| "error creating file for download")), 0) + }; + + let file = RefCell::new(file); + + try!(download_with_backend(backend, url, resume_from, &|event| { if let Event::DownloadDataReceived(data) = event { try!(file.borrow_mut().write_all(data) .chain_err(|| "unable to write download to disk")); @@ -89,11 +132,8 @@ pub fn download_to_path_with_backend( Ok(()) }().map_err(|e| { - if path.is_file() { - // FIXME ignoring compound errors - let _ = fs::remove_file(path); - } + // TODO is there any point clearing up here? What kind of errors will leave us with an unusable partial? e }) } @@ -114,6 +154,7 @@ pub mod curl { use super::Event; pub fn download(url: &Url, + resume_from: u64, callback: &Fn(Event) -> Result<()> ) -> Result<()> { // Fetch either a cached libcurl handle (which will preserve open @@ -128,6 +169,15 @@ pub mod curl { try!(handle.url(&url.to_string()).chain_err(|| "failed to set url")); try!(handle.follow_location(true).chain_err(|| "failed to set follow redirects")); + if resume_from > 0 { + try!(handle.resume_from(resume_from) + .chain_err(|| "setting the range header for download resumption")); + } else { + // an error here indicates that the range header isn't supported by underlying curl, + // so there's nothing to "clear" - safe to ignore this error. + let _ = handle.resume_from(0); + } + // Take at most 30s to connect try!(handle.connect_timeout(Duration::new(30, 0)).chain_err(|| "failed to set connect timeout")); @@ -154,8 +204,8 @@ pub mod curl { if let Ok(data) = str::from_utf8(header) { let prefix = "Content-Length: "; if data.starts_with(prefix) { - if let Ok(s) = data[prefix.len()..].trim().parse() { - let msg = Event::DownloadContentLengthReceived(s); + if let Ok(s) = data[prefix.len()..].trim().parse::() { + let msg = Event::DownloadContentLengthReceived(s + resume_from); match callback(msg) { Ok(()) => (), Err(e) => { @@ -188,11 +238,12 @@ pub mod curl { })); } - // If we didn't get a 200 or 0 ("OK" for files) then return an error + // If we didn't get a 20x or 0 ("OK" for files) then return an error let code = try!(handle.response_code().chain_err(|| "failed to get response code")); - if code != 200 && code != 0 { - return Err(ErrorKind::HttpStatus(code).into()); - } + match code { + 0 | 200 ... 299 => {}, + _ => { return Err(ErrorKind::HttpStatus(code).into()); } + }; Ok(()) }) @@ -639,6 +690,7 @@ pub mod curl { use super::Event; pub fn download(_url: &Url, + _resume_from: u64, _callback: &Fn(Event) -> Result<()> ) -> Result<()> { Err(ErrorKind::BackendUnavailable("curl").into()) diff --git a/src/download/tests/download-curl-resume.rs b/src/download/tests/download-curl-resume.rs new file mode 100644 index 0000000000..f28dd1b4ab --- /dev/null +++ b/src/download/tests/download-curl-resume.rs @@ -0,0 +1,98 @@ +#![cfg(feature = "curl-backend")] + +extern crate download; +extern crate tempdir; +extern crate url; + +use std::sync::{Arc, Mutex}; +use std::fs::{self, File}; +use std::io::{self, Read}; +use std::path::Path; + +use tempdir::TempDir; +use url::Url; + +use download::*; + +fn tmp_dir() -> TempDir { + TempDir::new("rustup-download-test-").expect("creating tempdir for test") +} + +fn file_contents(path: &Path) -> String { + let mut result = String::new(); + File::open(&path).unwrap().read_to_string(&mut result).expect("reading test result file"); + result +} + + +pub fn write_file(path: &Path, contents: &str) { + let mut file = fs::OpenOptions::new() + .write(true) + .truncate(true) + .create(true) + .open(path) + .expect("writing test data"); + + io::Write::write_all(&mut file, contents.as_bytes()).expect("writing test data"); + + file.sync_data().expect("writing test data"); +} + +#[test] +fn partially_downloaded_file_gets_resumed_from_byte_offset() { + let tmpdir = tmp_dir(); + let from_path = tmpdir.path().join("download-source"); + write_file(&from_path, "xxx45"); + + let target_path = tmpdir.path().join("downloaded"); + write_file(&target_path, "123"); + + let from_url = Url::from_file_path(&from_path).unwrap(); + download_to_path_with_backend( + Backend::Curl, + &from_url, + &target_path, + true, + None) + .expect("Test download failed"); + + assert_eq!(file_contents(&target_path), "12345"); +} + +#[test] +fn callback_gets_all_data_as_if_the_download_happened_all_at_once() { + let tmpdir = tmp_dir(); + + let from_path = tmpdir.path().join("download-source"); + write_file(&from_path, "xxx45"); + + let target_path = tmpdir.path().join("downloaded"); + write_file(&target_path, "123"); + + let from_url = Url::from_file_path(&from_path).unwrap(); + + let received_in_callback = Arc::new(Mutex::new(Vec::new())); + + download_to_path_with_backend(Backend::Curl, + &from_url, + &target_path, + true, + Some(&|msg| { + match msg { + Event::DownloadDataReceived(data) => { + for b in data.iter() { + received_in_callback.lock().unwrap().push(b.clone()); + } + } + _ => {} + } + + + Ok(()) + })) + .expect("Test download failed"); + + let ref observed_bytes = *received_in_callback.lock().unwrap(); + assert_eq!(observed_bytes, &vec![b'1', b'2', b'3', b'4', b'5']); + assert_eq!(file_contents(&target_path), "12345"); +} diff --git a/src/rustup-dist/src/dist.rs b/src/rustup-dist/src/dist.rs index f16499ac11..31520f8017 100644 --- a/src/rustup-dist/src/dist.rs +++ b/src/rustup-dist/src/dist.rs @@ -502,6 +502,23 @@ impl ops::Deref for File { } } +fn file_hash(path: &Path) -> Result { + let mut hasher = Sha256::new(); + use std::io::Read; + let mut downloaded = try!(fs::File::open(&path).chain_err(|| "opening already downloaded file")); + let mut buf = vec![0; 32768]; + loop { + if let Ok(n) = downloaded.read(&mut buf) { + if n == 0 { break; } + hasher.input(&buf[..n]); + } else { + break; + } + } + + Ok(hasher.result_str()) +} + impl<'a> DownloadCfg<'a> { pub fn download(&self, url: &Url, hash: &str, notify_handler: &'a Fn(Notification)) -> Result { @@ -510,19 +527,7 @@ impl<'a> DownloadCfg<'a> { let target_file = self.download_dir.join(Path::new(hash)); if target_file.exists() { - let mut hasher = Sha256::new(); - use std::io::Read; - let mut downloaded = try!(fs::File::open(&target_file).chain_err(|| "opening already downloaded file")); - let mut buf = [0; 1024]; - loop { - if let Ok(n) = downloaded.read(&mut buf) { - if n == 0 { break; } - hasher.input(&buf[..n]); - } else { - break; - } - } - let cached_result = hasher.result_str(); + let cached_result = try!(file_hash(&target_file)); if hash == cached_result { notify_handler(Notification::FileAlreadyDownloaded); notify_handler(Notification::ChecksumValid(&url.to_string())); @@ -533,11 +538,21 @@ impl<'a> DownloadCfg<'a> { } } + + let partial_file_path = + target_file.with_file_name( + target_file.file_name().map(|s| { + s.to_str().unwrap_or("_")}) + .unwrap_or("_") + .to_owned() + + ".partial"); + let mut hasher = Sha256::new(); - try!(utils::download_file(&url, - &target_file, + try!(utils::download_file_with_resume(&url, + &partial_file_path, Some(&mut hasher), + true, &|n| notify_handler(n.into()))); let actual_hash = hasher.result_str(); @@ -551,7 +566,8 @@ impl<'a> DownloadCfg<'a> { }.into()); } else { notify_handler(Notification::ChecksumValid(&url.to_string())); - return Ok(File { path: target_file, }) + try!(fs::rename(&partial_file_path, &target_file)); + return Ok(File { path: target_file }); } } diff --git a/src/rustup-dist/src/errors.rs b/src/rustup-dist/src/errors.rs index 5d34a6719b..5dc4e9cfaf 100644 --- a/src/rustup-dist/src/errors.rs +++ b/src/rustup-dist/src/errors.rs @@ -1,5 +1,5 @@ use std::path::PathBuf; -use std::io::Write; +use std::io::{self, Write}; use temp; use toml; use rustup_utils; @@ -12,6 +12,7 @@ error_chain! { foreign_links { Temp(temp::Error); + Io(io::Error); } errors { diff --git a/src/rustup-utils/src/notifications.rs b/src/rustup-utils/src/notifications.rs index ec1d3820a5..8356ad81a6 100644 --- a/src/rustup-utils/src/notifications.rs +++ b/src/rustup-utils/src/notifications.rs @@ -19,6 +19,7 @@ pub enum Notification<'a> { /// Download has finished. DownloadFinished, NoCanonicalPath(&'a Path), + ResumingPartialDownload, UsingCurl, UsingHyper, UsingRustls, @@ -35,6 +36,7 @@ impl<'a> Notification<'a> { DownloadContentLengthReceived(_) | DownloadDataReceived(_) | DownloadFinished | + ResumingPartialDownload | UsingCurl | UsingHyper | UsingRustls => NotificationLevel::Verbose, NoCanonicalPath(_) => NotificationLevel::Warn, } @@ -58,6 +60,7 @@ impl<'a> Display for Notification<'a> { DownloadDataReceived(data) => write!(f, "received some data of size {}", data.len()), DownloadFinished => write!(f, "download finished"), NoCanonicalPath(path) => write!(f, "could not canonicalize path: '{}'", path.display()), + ResumingPartialDownload => write!(f, "resuming partial download"), UsingCurl => write!(f, "downloading with curl"), UsingHyper => write!(f, "downloading with hyper + native_tls"), UsingRustls => write!(f, "downloading with hyper + rustls"), diff --git a/src/rustup-utils/src/utils.rs b/src/rustup-utils/src/utils.rs index b505b72689..d2d6da5629 100644 --- a/src/rustup-utils/src/utils.rs +++ b/src/rustup-utils/src/utils.rs @@ -144,8 +144,17 @@ pub fn download_file(url: &Url, hasher: Option<&mut Sha256>, notify_handler: &Fn(Notification)) -> Result<()> { + download_file_with_resume(&url, &path, hasher, false, ¬ify_handler) +} + +pub fn download_file_with_resume(url: &Url, + path: &Path, + hasher: Option<&mut Sha256>, + resume_from_partial: bool, + notify_handler: &Fn(Notification)) + -> Result<()> { use download::ErrorKind as DEK; - match download_file_(url, path, hasher, notify_handler) { + match download_file_(url, path, hasher, resume_from_partial, notify_handler) { Ok(_) => Ok(()), Err(e) => { let is_client_error = match e.kind() { @@ -171,6 +180,7 @@ pub fn download_file(url: &Url, fn download_file_(url: &Url, path: &Path, hasher: Option<&mut Sha256>, + resume_from_partial: bool, notify_handler: &Fn(Notification)) -> Result<()> { @@ -202,6 +212,9 @@ fn download_file_(url: &Url, Event::DownloadDataReceived(data) => { notify_handler(Notification::DownloadDataReceived(data)); } + Event::ResumingPartialDownload => { + notify_handler(Notification::ResumingPartialDownload); + } } Ok(()) @@ -218,7 +231,7 @@ fn download_file_(url: &Url, (Backend::Curl, Notification::UsingCurl) }; notify_handler(notification); - try!(download_to_path_with_backend(backend, url, path, Some(callback))); + try!(download_to_path_with_backend(backend, url, path, resume_from_partial, Some(callback))); notify_handler(Notification::DownloadFinished); @@ -413,7 +426,7 @@ pub fn home_dir() -> Option { let me = GetCurrentProcess(); let mut token = ptr::null_mut(); if OpenProcessToken(me, TOKEN_READ, &mut token) == 0 { - return None + return None; } let _g = scopeguard::guard(token, |h| { let _ = CloseHandle(*h); }); fill_utf16_buf(|buf, mut sz| {