3
\$\begingroup\$

My friend wrote a scraper in Go that takes the results from a house listing webpage and finds listings for houses that he's interested in. The initial search returns listings, they are filtered by city, and then the details of each listing is acquired from a separate HTTP request and more details are checked to see if it's an interesting listing. I also ported this over to Node.js.

I've written a version in Rust (version 1.6) that works, but I'm interested in a couple things:

  1. Am I doing this in a Rusty way or are there things I should be doing differently?
  2. Am I properly taking advantage of concurrency, specifically being able to parse multiple details pages at once? Particularly wondering how large to make the Pool.
  3. This version is several times slower than the Go/Node.js versions (they are both about the same speed). Is there something I'm missing that's taking up a lot of additional time? (using cargo run --release or just building for release).
  4. I am creating and passing in the redis client and http client to the check_block_and_parking function each time but it would make more sense not to pass these in but declare them globally or perhaps make them properties of an object. Suggestions?
  5. Is there any way to limit the number of simultaneous connections from the connection pool?

https://github.com/ajcrites/gainesvillemls-scraper-rust

extern crate scoped_threadpool;
extern crate hyper;
extern crate kuchiki;
extern crate redis;
extern crate num_cpus;

use hyper::Client;
use hyper::header::ContentType;
use kuchiki::traits::ParserExt;
use scoped_threadpool::Pool;
use redis::Commands;
use redis::Client as RedisClient;
use std::env;

const KEY: &'static str = "52633f4973cf845e55b18c8e22ab08d5";
const SEARCH_HOST: &'static str = "http://www.gainesvillemls.com";


fn main() {
    let http_client = &Client::new();
    let body = &format!("key={}&LM_MST_prop_fmtYNNT=1&LM_MST_prop_cdYYNT=1,9,10,11,12,13,14&LM_MST_mls_noYYNT=&LM_MST_list_prcYNNB=&LM_MST_list_prcYNNE=175000&LM_MST_prop_cdYNNL[]=9&LM_MST_sqft_nYNNB=&LM_MST_sqft_nYNNE=&LM_MST_yr_bltYNNB=&LM_MST_yr_bltYNNE=&LM_MST_bdrmsYNNB=3&LM_MST_bdrmsYNNE=&LM_MST_bathsYNNB=2&LM_MST_bathsYNNE=&LM_MST_hbathYNNB=&LM_MST_hbathYNNE=&LM_MST_countyYNCL[]=ALA&LM_MST_str_noY1CS=&LM_MST_str_namY1VZ=&LM_MST_remarksY1VZ=&openHouseStartDt_B=&openHouseStartDt_E=&ve_info=&ve_rgns=1&LM_MST_LATXX6I=&poi=&count=1&isLink=0&custom=", KEY);

    let redis_client = &redis::Client::open(&*env::var("REDIS_DSN").unwrap()).unwrap();

    let res = http_client.post(&format!("{}/gan/idx/search.php", SEARCH_HOST))
        .header(ContentType::form_url_encoded())
        .body(body)
        .send()
        .unwrap();

    let document = kuchiki::parse_html().from_http(res).unwrap();

    let cpus = num_cpus::get() * 4;
    let mut pool = Pool::new(cpus as u32);
    pool.scoped(|scope| {
        for listing in document.select("table.listings").unwrap() {
            let elem = listing.as_node();
            let text = elem.select("tr:nth-of-type(3)").unwrap().next().unwrap().text_contents();
            if text.to_lowercase().find("gainesville, fl") != None {
                let mls = elem.select("span.mls").unwrap().next().unwrap().text_contents();
                let price = elem.select("span.price").unwrap().next().unwrap().text_contents();
                scope.execute(move || {
                    check_block_and_parking(mls, price, http_client, redis_client);
                });
            }
        }
    });
}

fn check_block_and_parking(mls: String, price: String, http_client: &Client, redis_client: &RedisClient) {
    let redis_conn = redis_client.get_connection().unwrap();

    if redis_conn.hexists("mls", &*mls).unwrap() {
        return;
    }

    let _ : () = redis_conn.hset("mls", &*mls, &*price).unwrap();

    let res = http_client.post(&format!("{}/gan/idx/detail.php", SEARCH_HOST))
        .header(ContentType::form_url_encoded())
        .body(&format!("key={}&mls={}&gallery=false&custom=", KEY, mls))
        .send()
        .unwrap();

    let document = kuchiki::parse_html().from_http(res).unwrap();
    let mut has_parking = true;
    let mut has_block = false;

    for details in document.select("table.wide label.bold").unwrap() {
        if "Parking:" == details.text_contents() && details.as_node().parent().unwrap().select("span").unwrap().next().unwrap().text_contents().to_lowercase().find("no garage") != None {
            has_parking = false;
        }
        if "Construction-exterior:" == details.text_contents() && details.as_node().parent().unwrap().select("span").unwrap().next().unwrap().text_contents().to_lowercase().find("block") != None {
            has_block = true;
        }
    }

    if has_parking && has_block {
        println!("{}/gan/idx/index.php?key={}&mls={}", SEARCH_HOST, KEY, mls);
    }
}
[dependencies]
kuchiki = {version = "0.3.1", features = ["hyper"]}
hyper = "0.7.2"
scoped_threadpool = "0.1.7"
redis = "0.5.2"
num_cpus = "0.2.11"

I am compiling with cargo run --release.

\$\endgroup\$
3
\$\begingroup\$

Overall, the code looks close to how I would write it. A few smaller points:

  1. Use contains(...) instead of find(...) != None. It has the potential to be faster but more importantly it's more understandable and slightly shorter.
  2. You can use let () = instead of let _: () =. Both are pretty strange to see, but that appears to be an artifact of how flexible Redis is.
  3. The lines to look at the following span are really long and hard to see and are duplicated. I'd recommend creating a helper function.
  4. There are many places with unwrap. I can't really judge how valid it is to abort this entire program if things are missing, but the density of unwrap per line is much higher than I'm used to.
    1. You could combine items with functions like and_then. Unfotunately, the code often has to combine Result and Option, which makes this uglier.
    2. You could make more code conditional using match or if let statements.

Am I properly taking advantage of concurrency, specifically being able to parse multiple details pages at once? Particularly wondering how large to make the Pool.

Yes, it looks quite reasonable. In this application, I'd expect it to be more IO bound than CPU bound. There are good guides out there, but generally you'd want to have more working threads than you have CPUs as each thread is likely to be waiting for IO to happen.

This version is several times slower than the Go/Node.js versions (they are both about the same speed). Is there something I'm missing that's taking up a lot of additional time? (using cargo run --release or just building for release).

You didn't say how much time either of your versions took, so there's not much I can say here. Running the program locally took:

real    0m5.146s
user    0m1.139s
sys     0m0.678s

Which seems pretty fast.

I am creating and passing in the Redis client and HTTP client to the check_block_and_parking function each time but it would make more sense not to pass these in but declare them globally or perhaps make them properties of an object.

No, it would not make sense to make them global. A giant strength of Rust is that you can see where items are valid and have a very good sense of where memory will be allocated and deallocated. When you make globals, you lose that.

Additionally, global mutable items (which may not be needed here) are even more of a pain because they require a mutex or equivalent.

Making a new struct that holds other things could be useful though.

Is there any way to limit the number of simultaneous connections from the connection pool?

I'm not sure what you are asking here. You don't have a connection pool; you have a thread pool. Each thread makes HTTP connections, but these connections are not saved or reused. Currently, your maximum connection count is the number of threads.


extern crate scoped_threadpool;
extern crate hyper;
extern crate kuchiki;
extern crate redis;
extern crate num_cpus;

use hyper::Client;
use hyper::header::ContentType;
use kuchiki::{NodeDataRef, ElementData};
use kuchiki::traits::ParserExt;
use scoped_threadpool::Pool;
use redis::Commands;
use redis::Client as RedisClient;
use std::env;

const KEY: &'static str = "52633f4973cf845e55b18c8e22ab08d5";
const SEARCH_HOST: &'static str = "http://www.gainesvillemls.com";

fn main() {
    let http_client = &Client::new();
    let body = &format!("key={}&LM_MST_prop_fmtYNNT=1&LM_MST_prop_cdYYNT=1,9,10,11,12,13,14&LM_MST_mls_noYYNT=&LM_MST_list_prcYNNB=&LM_MST_list_prcYNNE=175000&LM_MST_prop_cdYNNL[]=9&LM_MST_sqft_nYNNB=&LM_MST_sqft_nYNNE=&LM_MST_yr_bltYNNB=&LM_MST_yr_bltYNNE=&LM_MST_bdrmsYNNB=3&LM_MST_bdrmsYNNE=&LM_MST_bathsYNNB=2&LM_MST_bathsYNNE=&LM_MST_hbathYNNB=&LM_MST_hbathYNNE=&LM_MST_countyYNCL[]=ALA&LM_MST_str_noY1CS=&LM_MST_str_namY1VZ=&LM_MST_remarksY1VZ=&openHouseStartDt_B=&openHouseStartDt_E=&ve_info=&ve_rgns=1&LM_MST_LATXX6I=&poi=&count=1&isLink=0&custom=", KEY);

    let redis_client = &redis::Client::open(&*env::var("REDIS_DSN").unwrap()).unwrap();

    let res = http_client.post(&format!("{}/gan/idx/search.php", SEARCH_HOST))
        .header(ContentType::form_url_encoded())
        .body(body)
        .send()
        .unwrap();

    let document = kuchiki::parse_html().from_http(res).unwrap();

    let cpus = num_cpus::get() * 4;
    let mut pool = Pool::new(cpus as u32);
    pool.scoped(|scope| {
        for listing in document.select("table.listings").unwrap() {
            let elem = listing.as_node();
            let text = elem.select("tr:nth-of-type(3)").unwrap().next().unwrap().text_contents();
            if text.to_lowercase().contains("gainesville, fl") {
                let mls = elem.select("span.mls").unwrap().next();
                let price = elem.select("span.price").unwrap().next();

                if let (Some(mls), Some(price)) = (mls, price) {
                    let mls = mls.text_contents();
                    let price = price.text_contents();

                    scope.execute(move || {
                        check_block_and_parking(mls, price, http_client, redis_client);
                    });
                }
            }
        }
    });
}

fn check_block_and_parking(mls: String, price: String, http_client: &Client, redis_client: &RedisClient) {
    let redis_conn = redis_client.get_connection().unwrap();

    if redis_conn.hexists("mls", &*mls).unwrap() {
        return;
    }

    let () = redis_conn.hset("mls", &*mls, &*price).unwrap();

    let res = http_client.post(&format!("{}/gan/idx/detail.php", SEARCH_HOST))
        .header(ContentType::form_url_encoded())
        .body(&format!("key={}&mls={}&gallery=false&custom=", KEY, mls))
        .send()
        .unwrap();

    let document = kuchiki::parse_html().from_http(res).unwrap();
    let mut has_parking = true;
    let mut has_block = false;

    fn name_value_is(details: &NodeDataRef<ElementData>, name: &str, value: &str) -> bool {
        if name != details.text_contents() { return false }

        let parent = details.as_node().parent().unwrap();
        let value_node = parent.select("span").unwrap().next().unwrap();

        value_node.text_contents().to_lowercase().contains(value)
    }

    for details in document.select("table.wide label.bold").unwrap() {
        if name_value_is(&details, "Parking:", "no garage") {
            has_parking = false;
        }
        if name_value_is(&details, "Construction-exterior:", "block") {
            has_block = true;
        }
    }

    if has_parking && has_block {
        println!("{}/gan/idx/index.php?key={}&mls={}", SEARCH_HOST, KEY, mls);
    }
}

Here's a version with almost all of the panicking paths removed. This was basically following the Error Handling section of The Rust Programming Language. I've left a few comments where I used different patterns:

extern crate scoped_threadpool;
extern crate hyper;
extern crate kuchiki;
extern crate redis;
extern crate num_cpus;

use hyper::Client;
use hyper::header::ContentType;
use kuchiki::{NodeDataRef, ElementData};
use kuchiki::traits::ParserExt;
use scoped_threadpool::Pool;
use redis::Commands;
use redis::Client as RedisClient;
use std::env;
use std::error::Error;

const KEY: &'static str = "52633f4973cf845e55b18c8e22ab08d5";
const SEARCH_HOST: &'static str = "http://www.gainesvillemls.com";

fn main() {
    if let Err(e) = inner_main() {
        println!("Processing failed: {}", e);
        std::process::exit(1);
    }
}

fn inner_main() -> Result<(), Box<Error>> {
    let http_client = &Client::new();
    let body = &format!("key={}&LM_MST_prop_fmtYNNT=1&LM_MST_prop_cdYYNT=1,9,10,11,12,13,14&LM_MST_mls_noYYNT=&LM_MST_list_prcYNNB=&LM_MST_list_prcYNNE=175000&LM_MST_prop_cdYNNL[]=9&LM_MST_sqft_nYNNB=&LM_MST_sqft_nYNNE=&LM_MST_yr_bltYNNB=&LM_MST_yr_bltYNNE=&LM_MST_bdrmsYNNB=3&LM_MST_bdrmsYNNE=&LM_MST_bathsYNNB=2&LM_MST_bathsYNNE=&LM_MST_hbathYNNB=&LM_MST_hbathYNNE=&LM_MST_countyYNCL[]=ALA&LM_MST_str_noY1CS=&LM_MST_str_namY1VZ=&LM_MST_remarksY1VZ=&openHouseStartDt_B=&openHouseStartDt_E=&ve_info=&ve_rgns=1&LM_MST_LATXX6I=&poi=&count=1&isLink=0&custom=", KEY);

    let dsn = try!(env::var("REDIS_DSN"));
    let redis_client = &try!(redis::Client::open(&*dsn));

    let res = try!(http_client.post(&format!("{}/gan/idx/search.php", SEARCH_HOST))
        .header(ContentType::form_url_encoded())
        .body(body)
        .send());

    let document = try!(kuchiki::parse_html().from_http(res));

    let cpus = num_cpus::get() * 4;
    let mut pool = Pool::new(cpus as u32);

    pool.scoped(|scope| {
        let listings = try!(document.select("table.listings").map_err(|_| "Could not select listings"));
        for listing in listings {
            let elem = listing.as_node();
            if let Some(text) = try!(elem.select("tr:nth-of-type(3)").map_err(|_| "Could not select text")).next() {
                let text = text.text_contents();

                if text.to_lowercase().contains("gainesville, fl") {
                    let mls = try!(elem.select("span.mls").map_err(|_| "Could not select mls")).next();
                    let price = try!(elem.select("span.price").map_err(|_| "Could not select price")).next();

                    // Using if let
                    if let (Some(mls), Some(price)) = (mls, price) {
                        let mls = mls.text_contents();
                        let price = price.text_contents();

                        scope.execute(move || {
                            check_block_and_parking(mls, price, http_client, redis_client).expect("The inner thread failed");
                        });
                    }
                }
            }
        }

        Ok(())
    })
}

fn check_block_and_parking(mls: String, price: String, http_client: &Client, redis_client: &RedisClient) -> Result<(), Box<Error>> {
    let redis_conn = try!(redis_client.get_connection());

    if try!(redis_conn.hexists("mls", &*mls)) {
        return Ok(());
    }

    let () = try!(redis_conn.hset("mls", &*mls, &*price));

    let res = try!(http_client.post(&format!("{}/gan/idx/detail.php", SEARCH_HOST))
        .header(ContentType::form_url_encoded())
        .body(&format!("key={}&mls={}&gallery=false&custom=", KEY, mls))
        .send());

    let document = try!(kuchiki::parse_html().from_http(res));
    let mut has_parking = true;
    let mut has_block = false;

    fn name_value_is(details: &NodeDataRef<ElementData>, name: &str, value: &str) -> Result<bool, Box<Error>> {
        if name != details.text_contents() { return Ok(false) }

        // Using a match
        let found = match details.as_node().parent() {
            Some(parent) => {
                // using map_err
                let mut value_node = try!(parent.select("span").map_err(|_| "Could not select span children"));

                // Using map_or
                value_node.next().map_or(false, |n| {
                    n.text_contents().to_lowercase().contains(value)
                })
            },
            None => false,
        };

        Ok(found)
    }

    let details = try!(document.select("table.wide label.bold").map_err(|_| "Could not select attributes"));

    for detail in details {
        if try!(name_value_is(&detail, "Parking:", "no garage")) {
            has_parking = false;
        }
        if try!(name_value_is(&detail, "Construction-exterior:", "block")) {
            has_block = true;
        }
    }

    if has_parking && has_block {
        println!("{}/gan/idx/index.php?key={}&mls={}", SEARCH_HOST, KEY, mls);
    }

    Ok(())
}

However, only you can decide what errors are appropriate to return up to the caller and which indicate a problem that the programmer has made. For example, each select call has a hard-coded selector. If the selector is malformed, that's probably a programmer error. If some data is missing, that's more likely to be user error.

One thing that is ugly here is that select returns an Result<..., ()>, but () doesn't play nicely with any of the Error machinery. That's why I had to use map_err to transform it. You may wish to inform the crate maintainers that that isn't very ergonomic.

\$\endgroup\$
  • \$\begingroup\$ Thanks, this is great. Can you offer a couple of examples of and_then or if let? I sort of understand what you mean about the .unwrap and I will do my own research -- I'm used to being able to use something like catch in cases like this. re: connection pool I was referring specifically to Redis since I have a limit on my Redis account. The http connection pool should be fine ... limited by the OS but the underlying http driver probably handles that. \$\endgroup\$ – Explosion Pills Feb 29 '16 at 22:50
  • 1
    \$\begingroup\$ @ExplosionPills I made some updates \$\endgroup\$ – Shepmaster Mar 1 '16 at 4:03
2
\$\begingroup\$

Your code has a lot of rightward drift. This is even worse in Shepmaster's alterations, which I advise you take.

pool.scoped(|scope| {
    let listings = try!(document.select("table.listings").map_err(|_| "Could not select listings"));
    for listing in listings {
        let elem = listing.as_node();
        if let Some(text) = try!(elem.select("tr:nth-of-type(3)").map_err(|_| "Could not select text")).next() {
            let text = text.text_contents();

            if text.to_lowercase().contains("gainesville, fl") {
                let mls = try!(elem.select("span.mls").map_err(|_| "Could not select mls")).next();
                let price = try!(elem.select("span.price").map_err(|_| "Could not select price")).next();

                // Using if let
                if let (Some(mls), Some(price)) = (mls, price) {
                    let mls = mls.text_contents();
                    let price = price.text_contents();

                    scope.execute(move || {
                        check_block_and_parking(mls, price, http_client, redis_client).expect("The inner thread failed");
                    });
                }
            }
        }
    }

    Ok(())
})

One way of improving this is to early-out your ifs and if lets. I would also separate some of the longer lines. For example, I would try something like (untested):

pool.scoped(|scope| {
    let listings = document.select("table.listings")
                           .map_err(|_| "Could not select listings");

    for listing in try!(listings) {
        let elem = listing.as_node();

        let third_row = elem.select("tr:nth-of-type(3)")
                            .map_err(|_| "Could not select text");
        let text = match try!(third_row).next() {
            Some(text) => text,
            None => continue,
        };
        if !text.text_contents().to_lowercase().contains("gainesville, fl") {
            continue;
        }

        let mls = elem.select("span.mls").map_err(|_| "Could not select mls");
        let price = elem.select("span.price").map_err(|_| "Could not select price");

        if let (Some(mls), Some(price)) = (try!(mls).next(), try!(price).next()) {
            let mls = mls.text_contents();
            let price = price.text_contents();

            scope.execute(move || {
                check_block_and_parking(mls, price, http_client, redis_client)
                    .expect("The inner thread failed");
            });
        }
    }

    Ok(())
})
\$\endgroup\$
  • \$\begingroup\$ I agree that rightward drift is quite a nuisance, and it's a good idea to address it. Pulling out methods tends to be my primary solution to rightward drift. Not sure I agree with the multi-line match guard though; that took me a few seconds to understand. \$\endgroup\$ – Shepmaster Mar 5 '16 at 20:49
  • \$\begingroup\$ @Shepmaster I kind'a agree about the match, so I've replaced it with a simpler 2-step version. \$\endgroup\$ – Veedrac Mar 5 '16 at 23:02

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.