-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Resumable downloads. #84
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1st part of my review, trying to figure out how things work in tokio
let new_size = (size as f32 * truncate) as u64; | ||
file.set_len(new_size).unwrap(); | ||
let mut blob_part = blob.clone(); | ||
blob_part.set_extension(".part"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use EXTENTION
here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's done on purpose to have it hardcoded in the test. I don't want my test to change when library code changes.
|
||
let mut res = self.download_from(url, 0u64, size, &mut file, filename, &mut progress); | ||
let mut file = match std::fs::OpenOptions::new().append(true).open(&filepath) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let mut file = match std::fs::OpenOptions::new().append(true).open(&filepath) { | |
let mut part_file = match std::fs::OpenOptions::new().append(true).open(&filepath) { |
src/api/tokio.rs
Outdated
.await | ||
{ | ||
Ok(mut f) => { | ||
let len = f.metadata().await.unwrap().len(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unwrap
expected here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
0 | ||
} | ||
} | ||
Err(_err) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Err(_err) => { | |
Err(_) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, understood!
Notes on the strategy here.
Sync:
Tokio:
Since we're multiplexing, writes are not happening in order. We could:
u64
at the very end of the partial file. When resuming, we read that single int, and resume from there. Anything fishy and we drop the resumability.This feels simpler that the more complex version, seems to work quite well in practice (only tested on stable networks right now).
The probability for storing invalid files seems low (since commit happens at a single location both in code and in file).