2
\$\begingroup\$

This is likely my first useful piece of Rust code. Planning to crate-ify it. It works.

#[macro_use]
extern crate failure;

use std::collections::HashMap;
use std::fs::File;
use std::io::prelude::*;
use std::path::Path;
use std::env::temp_dir;

use mio::{Events, Poll};
use mio_httpc::{CallBuilder, Httpc, HttpcCfg, SimpleCall};

use url::Url;

use failure::Error;

fn do_call(htp: &mut Httpc, poll: &Poll, mut call: SimpleCall) -> Result<String, Error> {
    let mut buf: Option<String>;
    let to = ::std::time::Duration::from_millis(100);
    let mut events = Events::with_capacity(8);
    'outer: loop {
        poll.poll(&mut events, Some(to)).unwrap();
        for cref in htp.timeout().into_iter() {
            if call.is_ref(cref) {
                return Err(format_err!("Request timed out"));
            }
        }

        for ev in events.iter() {
            let cref = htp.event(&ev);

            if call.is_call(&cref) && call.perform(htp, &poll).expect("Call failed") {
                let (resp, body) = call.finish().expect("No response");
                println!("done req = {}", resp.status);
                for h in resp.headers() {
                    println!("Header = {}", h);
                }
                match String::from_utf8(body.clone()) {
                    Ok(s) => buf = Some(s),
                    Err(_) => {
                        return Err(format_err!("Non utf8 body sized: {}", body.len()));
                    }
                }
                break 'outer;
            }
        }
    }
    match buf {
        Some(s) => Ok(s),
        None => Err(format_err!("Empty response")),
    }
}

pub fn basename(path: &str) -> String {
    match path.rsplit(std::path::MAIN_SEPARATOR).next() {
        Some(p) => p.to_string(),
        None => path.into(),
    }
}

pub fn download(dir: Option<&str>, urls: Vec<&str>) -> Result<HashMap<String, String>, Error> {
    let mut url2response: HashMap<String, String> = HashMap::new();

    let poll = Poll::new()?;

    let cfg = match HttpcCfg::certs_from_path(".") {
        Ok(cfg) => cfg,
        Err(_) => Default::default(),
    };
    let mut htp = Httpc::new(10, Some(cfg));

    for i in 0..urls.len() {
        let call = CallBuilder::get()
            .url(urls[i])? // .expect("Invalid url")
            .timeout_ms(10000)
            // .insecure_do_not_verify_domain()
            .simple_call(&mut htp, &poll)?; // .expect("Call start failed");

        let mut error: Option<Error> = None;
        let download_path: Option<String> = if dir.is_some() {
            let uri_opt = match Url::parse(urls[i]) {
                Ok(uri) => Some(uri),
                Err(e) => {
                    error = Some(format_err!("{}", e.to_string()));
                    None
                }
            };
            if uri_opt.is_none() {
                None
            } else {
                let p = Path::new(&dir.unwrap())
                    .join(Path::new(&basename(uri_opt.unwrap().path())))
                    .to_string_lossy()
                    .into_owned();
                if p.is_empty() {
                    error = Some(format_err!(
                        "Conversion to filename failed for: {:#?}",
                        urls[i]
                    ));
                    None
                } else {
                    Some(p)
                }
            }
        } else {
            None
        };

        if error.is_some() {
            return Err(error.unwrap());
        }

        if download_path.is_none() && dir.is_some() {
            return Err(format_err!("No filename detectable from URL"));
        }

        match do_call(&mut htp, &poll, call) {
            Ok(response_string) => url2response.insert(
                urls[i].into(),
                String::from(if dir.is_some() {
                    let dp = download_path.unwrap();
                    write_file(Path::new(&dp), response_string)?;
                    dp
                } else {
                    response_string
                }),
            ),
            Err(e) => return Err(format_err!("{}", e)),
        };

        println!("Open connections = {}", htp.open_connections());
    }

    Ok(url2response)
}

pub fn write_file(path: &Path, content: String) -> Result<(), Error> {
    let display = path.display();
    let mut file = match File::create(&path) {
        Err(why) => Err(format_err!(
            "couldn't create {}: {:#?}",
            display,
            why.kind()
        )),
        Ok(file) => Ok(file),
    }?;
    match file.write_all(content.as_bytes()) {
        Err(why) => Err(format_err!(
            "couldn't write to {}: {:#?}",
            display,
            why.kind()
        )),
        Ok(_) => Ok(()),
    }
}

pub fn env_or(key: &str, default: std::ffi::OsString) -> std::ffi::OsString {
    match std::env::var_os(key) {
        Some(val) => val,
        None => default,
    }
}

#[cfg(test)]
mod tests {
    use super::*;

    const URLS: &'static [&'static str] = &["http://detectportal.firefox.com/success.txt"];

    #[test]
    fn download_to_dir() {
        let _td = temp_dir();
        let _td_cow = _td.to_string_lossy();
        let tmp_dir = _td_cow.as_ref();
        match download(Some(tmp_dir), URLS.to_vec()) {
            Ok(url2response) => {
                for (url, response) in &url2response {
                    assert_eq!(url, URLS[0]);
                    assert_eq!(Path::new(response), Path::new(tmp_dir).join("success.txt"))
                }
            }
            Err(e) => panic!(e),
        }
    }

    #[test]
    fn download_to_mem() {
        match download(None, URLS.to_vec()) {
            Ok(url2response) => {
                for (url, response) in &url2response {
                    assert_eq!(url, URLS[0]);
                    assert_eq!(response, "success\n")
                }
            }
            Err(e) => panic!(e),
        }
    }
}

Annoyed though, it's ~200 lines of code, not particularly readable, lots of type conversions, not sure if I should be:

  • borrowing and using implicit conversions with From on function definitions and/or AsRef to enable different kinds of collections;
    • E.g.: can I support arrays, slices, queues, vectors and stacks? - maybe rewrite using Iterator?
  • relying on rayon as well as mio_httpc, so I can get simple multithreading and multiprocessing
  • using async/await and integrating that into mio_httpc to cleanup the code
  • error handling in some better way?
    • On that note, I guess I should test error branches also?
  • what else I could be doing better?
\$\endgroup\$
0
\$\begingroup\$

Error Handling

Your error handling is pretty inconsistent. Sometimes you just unwrap:

    poll.poll(&mut events, Some(to)).unwrap();

Sometimes you use expect:

    call.perform(htp, &poll).expect("Call failed")

Sometimes you use ?:

    let poll = Poll::new()?;

Sometimes you match:

    match do_call(&mut htp, &poll, call) {
        Ok(response_string) => ...
        Err(e) => return Err(format_err!("{}", e)),
    };

If you plan to write a reusable crate, you certainly do not want to panic!() via unwrap() or expect(). Your code in general would be a lot simpler if it consistently used ? to handle errors.

Types

Right now you pass around strings a lot. Part of the advantage of Rust is a powerful type system. Where possible it is better to have code take more precise types as parameters. For example, instead of taking the urls as a list of strings, take a list of URLs. Instead of running a path as a String, return it as a Path. When it comes download files from URLS, there is a good chance you don't want Strings at all (which have to be utf-8) but the more generic Vec<u8>

Api

Your download function does very different things depending on whether the dir is None or not. In particular, the return value has a completely different meaning. This sort of thing is tolerated in some other languages, but deeply frowned upon in Rust. Downloading to memory and to file should be different functions.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.