Merge pull request #1238 from seanmonstar/reqwest

improve reqwest backend

Author: alexcrichton

Cargo.lock

@@ -11,14 +11,17 @@ license = "MIT/Apache-2.0"
 default = ["curl-backend"]
 
 curl-backend = ["curl"]
-reqwest-backend = ["reqwest", "env_proxy"]
+reqwest-backend = ["reqwest", "env_proxy", "lazy_static"]
 
 [dependencies]
 error-chain = "0.10"
 url = "1.2"
 curl = { version = "0.4.6", optional = true }
 env_proxy = { version = "0.1.1", optional = true }
-reqwest = { version = "0.7.2", optional = true }
+lazy_static = { version = "0.1", optional = true }
+reqwest = { version = "0.7.3", optional = true }
 
 [dev-dependencies]
+futures = "0.1"
+hyper = "0.11"
 tempdir = "0.3.4"

src/download/Cargo.toml

@@ -6,6 +6,9 @@ extern crate url;
 
 #[cfg(feature = "reqwest-backend")]
 extern crate reqwest;
+#[cfg(feature = "reqwest-backend")]
+#[macro_use]
+extern crate lazy_static;
 
 use url::Url;
 use std::path::Path;
@@ -237,56 +240,24 @@ pub mod reqwest_be {
     extern crate env_proxy;
 
     use std::io;
+    use std::time::Duration;
     use errors::*;
     use url::Url;
     use super::Event;
-    use reqwest::{header, Client, Proxy};
+    use reqwest::{header, Client, Proxy, Response};
 
     pub fn download(url: &Url,
                     resume_from: u64,
                     callback: &Fn(Event) -> Result<()>)
                     -> Result<()> {
 
         // Short-circuit reqwest for the "file:" URL scheme
-        if try!(download_from_file_url(url, callback)) {
+        if download_from_file_url(url, resume_from, callback)? {
             return Ok(());
         }
 
-        let maybe_proxy = env_proxy::for_url(url)
-            .map(|(addr, port)| {
-                String::from(format!("{}:{}", addr, port))
-            });
-
-        let client = match url.scheme() {
-            "https" => match maybe_proxy {
-                None => Client::new()?,
-                Some(host_port) => {
-                    Client::builder()?
-                        .proxy(Proxy::https(&host_port)?)
-                        .build()?
-                }
-            },
-            "http" => match maybe_proxy {
-                None => Client::new()?,
-                Some(host_port) => {
-                    Client::builder()?
-                        .proxy(Proxy::http(&host_port)?)
-                        .build()?
-                }
-            },
-            _ => return Err(format!("unsupported URL scheme: '{}'", url.scheme()).into())
-        };
-
-        let mut res = if resume_from > 0 {
-            client
-                .get(url.clone())?
-                .header(header::Range::Bytes(
-                    vec![header::ByteRangeSpec::AllFrom(resume_from)]
-                ))
-                .send()
-        } else {
-            client.get(url.clone())?.send()
-        }.chain_err(|| "failed to make network request")?;
+        let mut res = request(url, resume_from)
+            .chain_err(|| "failed to make network request")?;
 
         if !res.status().is_success() {
             let code: u16 = res.status().into();
@@ -296,23 +267,65 @@ pub mod reqwest_be {
         let buffer_size = 0x10000;
         let mut buffer = vec![0u8; buffer_size];
 
-        if let Some(len) = res.headers().get::<header::ContentLength>().cloned() {
-            try!(callback(Event::DownloadContentLengthReceived(len.0)));
+        if let Some(len) = res.headers().get::<header::ContentLength>() {
+            callback(Event::DownloadContentLengthReceived(len.0 + resume_from))?;
         }
 
         loop {
-            let bytes_read = try!(io::Read::read(&mut res, &mut buffer)
-                                  .chain_err(|| "error reading from socket"));
+            let bytes_read = io::Read::read(&mut res, &mut buffer)
+                .chain_err(|| "error reading from socket")?;
 
             if bytes_read != 0 {
-                try!(callback(Event::DownloadDataReceived(&buffer[0..bytes_read])));
+                callback(Event::DownloadDataReceived(&buffer[0..bytes_read]))?;
             } else {
                 return Ok(());
             }
         }
     }
 
+    lazy_static! {
+        static ref CLIENT: Client = {
+            let catcher = || {
+                Client::builder()?
+                    .gzip(false)
+                    .proxy(Proxy::custom(env_proxy))
+                    .timeout(Duration::from_secs(30))
+                    .build()
+            };
+
+            // woah, an unwrap?!
+            // It's OK. This is the same as what is happening in curl.
+            //
+            // The curl::Easy::new() internally assert!s that the initialized
+            // Easy is not null. Inside reqwest, the errors here would be from
+            // the TLS library returning a null pointer as well.
+            catcher().unwrap()
+        };
+    }
+
+    fn env_proxy(url: &Url) -> Option<Url> {
+        env_proxy::for_url(url).and_then(|(host, port)| {
+            //TODO: update env_proxy to return full string, not just (host,port)
+            //Ideally: fn for_str(s: &str) -> Option<String>
+            let proxy_url = format!("http://{}:{}", host, port);
+            proxy_url.parse().ok()
+        })
+    }
+
+    fn request(url: &Url, resume_from: u64) -> ::reqwest::Result<Response> {
+        let mut req = CLIENT.get(url.clone())?;
+
+        if resume_from != 0 {
+            req.header(header::Range::Bytes(
+                vec![header::ByteRangeSpec::AllFrom(resume_from)]
+            ));
+        }
+
+        req.send()
+    }
+
     fn download_from_file_url(url: &Url,
+                              resume_from: u64,
                               callback: &Fn(Event) -> Result<()>)
                               -> Result<bool> {
 
@@ -333,6 +346,7 @@ pub mod reqwest_be {
 
             let ref mut f = try!(fs::File::open(src)
                                  .chain_err(|| "unable to open downloaded file"));
+            io::Seek::seek(f, io::SeekFrom::Start(resume_from))?;
 
             let ref mut buffer = vec![0u8; 0x10000];
             loop {

src/download/src/lib.rs

@@ -1,42 +1,16 @@
 #![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 std::sync::Mutex;
 
-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");
-}
+mod support;
+use support::{tmp_dir, write_file, file_contents, serve_file};
 
 #[test]
 fn partially_downloaded_file_gets_resumed_from_byte_offset() {
@@ -62,36 +36,47 @@ fn partially_downloaded_file_gets_resumed_from_byte_offset() {
 #[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 (addr, _shutdown) = serve_file(b"xxx45".to_vec());
+
+    let from_url = format!("http://{}", addr).parse().unwrap();
 
-    let received_in_callback = Arc::new(Mutex::new(Vec::new()));
+    let callback_partial = Mutex::new(false);
+    let callback_len = Mutex::new(None);
+    let received_in_callback = Mutex::new(Vec::new());
 
     download_to_path_with_backend(Backend::Curl,
                                   &from_url,
                                   &target_path,
                                   true,
                                   Some(&|msg| {
         match msg {
+            Event::ResumingPartialDownload => {
+                let mut flag = callback_partial.lock().unwrap();
+                assert!(!*flag);
+                *flag = true;
+            },
+            Event::DownloadContentLengthReceived(len) => {
+                let mut flag = callback_len.lock().unwrap();
+                assert!(flag.is_none());
+                *flag = Some(len);
+            }
             Event::DownloadDataReceived(data) => {
                 for b in data.iter() {
                     received_in_callback.lock().unwrap().push(b.clone());
                 }
             }
-            _ => {}
         }
 
 
         Ok(())
     }))
             .expect("Test download failed");
 
+    assert!(*callback_partial.lock().unwrap());
+    assert_eq!(*callback_len.lock().unwrap(), Some(5));
     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");