Auto merge of #1020 - jelford:support_partial_downloads, r=brson

Support partial downloads

This PR should close  #889

A couple of implementation details:

Only added support for the curl backend; previously discussed that there's an intention to get rid of rustup's own download code, and the default feature-set uses curl anyway, so hopefully this is okay.

Added new testing to the download crate - while it's there, it makes sense to have a test. Since using curl's "resume" functionality, I figured it's probably fine to just file:// urls for test cases. Previously tested using a small hyper-based http server, but that feels like overkill.

For hashing files, I've set the buffer size to 2^15 - just because that's what strace tells me is used by `sha256sum` on my local PC. It seems much slower than that command though, and it's not obvious why, so maybe I've done something silly here.

Finally, and maybe most controversially, I haven't done anything about cleaning up aborted partials. I don't really know when a good time is to do this, but a couple of suggestions that I'd be happy to implement:
* Every run, just check the download cache for any files > 7 days old and smoke them
* On self-update, as that seems like a natural time for generic "maintenance" sorts of operations

I mentioned in my last PR, but the same disclaimer: I haven't written much rust, so I fully expect you will see some problems (also very happy to accept style criticisms). I accidentally ran a `rustfmt` on some things so apologies for the noise (can revert but... maybe it's worth having anyway?).

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

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

src/download/src/errors.rs

@@ -1,7 +1,11 @@
+
+
 error_chain! {
     links { }
 
-    foreign_links { }
+    foreign_links {
+        Io(::std::io::Error);
+    }
 
     errors {
         HttpStatus(e: u32) {

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::<u64>() {
+                                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())

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");
+}