Add "Make a partial download with HTTP range headers" example #291

[jbolila]

This is my first PR in a Rust project, probably not the most idiomatic Rust but I'm here to learn.

[budziq]

@jbolila Thanks, the example looks almost perfect! Please find some minor suggestion below

[budziq]

@jbolila Thanks, the example looks almost perfect! Please find some minor suggestion below

[budziq]

instead of if let i would suggest using ok_or combinator to convert into a Result and immediately ?

[budziq]

please also update the intro.md

[budziq]

i would suggest using fixed buffer size instead of chunk number. Otherwise things are going to get weird with small content sizes and there is no reason to request exactly n times.

[budziq]

please use std before tbe crate uses

[budziq]

we can drop the if let and unwrap_or here
with something like:

        let content_length = match **range {
            ContentRangeSpec::Bytes { instance_length: Some(len), .. } => len,
            _ => bail!("response doesn't include the expected ranges"),
        };

[budziq]

please drop the assert_eq here.

[budziq]

we have just bumped to reqwest 0.8 which uses Result more sparingly so this will not compile now/ the same for head and get calls below

[budziq]

reqwest uses Url::parse internally so basically below will work

    let url = "https://httpbin.org/range/1024?duration=2";
    let response = client.head(url).send()?;

without the need for all the additional to_owned calls below

[budziq]

please use wildcard version * in docs.rs links (please also update any other links with hardcoded version that you find in this file)

[budziq]

I would mention about getting the ContentRange with head request.

[jbolila]

Thank you @budziq for your comments, realy appreciated. I'm no sure about the rebase I did in my fork to get the latest changes and the update of the Cargo.toml .. (i don't have much experience with PR)

[budziq]

@jbolila It seams that there was a problem with the rebase. You have pulled and rebased some of the already merged commits making this PR not mergable.
I would suggest backtracking a little.

git remote add upstream git@github.com:rust-lang-nursery/rust-cookbook.git
git remote update                       # get latest changes
git checkout f46683b -b jbolila_pr_fix  # get your original commit
git rebase upstream/master              # place it on top of current master
git cherry-pick b1bf49dec8              # add your latest fixes

then in the branch "jbolila_pr_fix" you'll have the state that you will want to push to your origin/master (with --force) to update the PR into a proper condition.

Then we will continue with the review.
If you'll have any further problems, do not hesitate to ask.

[jbolila]

I hope is right, now.

*   a0370ed (HEAD -> master) Merge branch 'jbolila_pr_fix'
|\  
| * fcacb9f (jbolila_pr_fix) Changes requested in the PR 309
| * 4e675fb Add "Make a partial download with HTTP range headers" example #291
| * b7ab129 (upstream/master) Parse a complex version string.
| * 6ffdf8e Minor fixes
..

[budziq]

unfortunately not yet. instead of merging jbolila_fix_pr you should overwrite master with it.

git checkout jbolila_fix_pr
git branch -D master
git checkout -b master
git push -u origin master --force

[budziq]

@jbolila nice! Just few suggestions and we are ready to merge!

[budziq]

@jbolila nice! Just few suggestions and we are ready to merge!

[budziq]

Although I would suggest not to hardcode "100" here but have some value/constant indicating the buffer size

Also are you upto converting the offset calculation code into an iterator returning Range::bytes? That would be awesome!

[jbolila]

I can try that one, although looks like in the std Range::step_by() is still in the nightly build only #27741. I'll do it for now manually inside the for loop.

[budziq]

the step_by would not help here much as it would just return every N'th value of the range (treated as iterator here) so

 for s in (0..1024).step_by(100) {
    println!("{:?}", s);
 }

would give

0
100
200
300
400
500
600
700
800
900
1000

Which would help with the implementation but would not give a full solution.
I would propose to make own iterator returning
hyper::header::Range::bytes with each iteration containing the next range we want to obtain

[budziq]

Anyhow, if you'll have any problems or concerns I'll gladly help!

[jbolila]

@budziq please let me know if it's ok, and if you wanna hide some of these extra lines. Thanks!

[budziq]

most of the time when we use partial downloading it's due to the fact that we either want to resume the download later or the whole content is to larg to hold in a contiguous memory buffer.

I would suggest using content buffer length as the each partial content request length (upto the last possibly shorter one). and each time writing it into a file with given offset.

[jbolila]

I'll use the value of the constant to set the size of the buffer.

What will be the filename? wouldn't that be to much for this example, I'm just concerned about the amount of code for the reqwest/ranges compared to the file handle.

[budziq]

opening and writing to a file would be only two additional lines which we could hide with # anyway. the filename can be anything like "download.bin"

[budziq]

@jbolila Nice!. I've left few suggestions to make the example more concise. This will probably be the last round of review before merge!

[budziq]

@jbolila Nice!. I've left few suggestions to make the example more concise. This will probably be the last round of review before merge!

[budziq]

no need to allocate the buffer on each iteration. Lets allocate it before the loop and use Vec::clear after each iteration

[budziq]

Id suggest using equivalent boolean expression that might be easier to read
(!a && !b) => !(a || b)

[jbolila]

I'll do it as you wish, although for me it's not easier to read this way.

        if !(response.status() == reqwest::StatusCode::PartialContent ||
                 response.status() == reqwest::StatusCode::Ok)
        {
            bail!("Unexpected server response: {}", response.status())
        }

[budziq]

the example is no_run anyway. we can go wild with the buffer and request size
the biggest request handled by httpbin seams to be around 102400 but I did not check precisely. The buffer can be set to something like 10KB to still show partial content (this is still unreasonably small. we might have to add a comment that normally it would be bigger )

[budziq]

I propose to merge the PartialRange and PartialRangeIter into one which will be much more concise.

All we will need will be:

• the struct

struct PartialRangeIter {
    start: u64,  // content range start
    end: u64,   // content range end
    buffer_size: usize, // external buffer size 
}

• The constructor new function below
• iter impl which will be much simplified

We should not need anything more complicated than

impl Iterator for PartialRangeIter {
    type Item = Range;

    fn next(&mut self) -> Option<Self::Item> {
        if self.start >= self.end {
            None
        } else {
            let old_start = self.start;
            self.start += cmp::min(self.buffer_size as u64, self.end - self.start +1);
            Some(Range::bytes(old_start, self.start -1))
        }
    }
}

[jbolila]

I have tried that approach during this end up with the following error, and now is back again with this code you're posting here:

error[E0599]: no method named `iter` found for type `PartialRangeIter` in the current scope
  --> src/main.rs:77:25
   |
77 |     for range in ranges.iter() {
   |                         ^^^^

here is the complete struct + impl:

#[derive(Debug)]
struct PartialRangeIter {
    start: u64, // content range start
    end: u64, // content range end
    buffer_size: usize, // external buffer size
}

impl PartialRangeIter {
    pub fn new(content_range: &ContentRangeSpec, buffer_size: usize) -> Result<PartialRangeIter> {
        if buffer_size == 0 {
            Err("invalid buffer_size, give a value greater than zero.")?;
        }

        match *content_range {
            ContentRangeSpec::Bytes { range: Some(range), .. } => Ok(PartialRangeIter {
                start: range.0,
                end: range.1,
                buffer_size,
            }),
            _ => Err("invalid range specification")?,
        }
    }
}

impl Iterator for PartialRangeIter {
    type Item = Range;

    fn next(&mut self) -> Option<Self::Item> {
        if self.start >= self.end {
            None
        } else {
            let old_start = self.start;
            self.start += std::cmp::min(self.buffer_size as u64, self.end - self.start + 1);
            Some(Range::bytes(old_start, self.start - 1))
        }
    }
}

can you explain?

[budziq]

for range in ranges.iter() {

There is nothing to provide the iter method nor is it required.
Som types provide iter, into_iter or both to return a separate helper iterator type.
In this situation we already have an iterator type instance as it can be constructed with PartialRangeIter::new (not being tied to any other type).

So following would simply work

for range in ranges {

I hope that helpes 👍

[budziq]

anything below here upto the run function will not be neaded

[budziq]

the iterator with (0,0,0) does not make sense/ would be invalid so I would suggest returning Err("invalid range speccification")? in this match leg,

[budziq]

this looks like a failable operation I would suggest returning Result<PartialRangeIter> from this function

also we will need the buffer_size as an argument. I would strongly advise against using global constants throughout the code. This leads to bad refactoring experience.

we will also need to check the buffer_size for being > 0 and return Err if it is not

[budziq]

how about directly constructing the Ok(PartialRangeIter{....}) here and leaving the match as the last statement so it is naturally returned from the function. If it will be to dense, we can always return to the parameter extraction in match and returning after it.

[budziq]

Merged, Excellent work @jbolila !
I've squashed the commits and I'll post minor cleanup shortly.

[jbolila]

Thank you @budziq !! I have learned a few things during this days, thanks for your patience and guidance.

[budziq]

No problem! That is exactly what this effort is for!