Draft: Download manager #7
Conversation
cyberphantom52
requested review from
jntesteves,
mirkobrombin,
edfloreshz and
TheEvilSkeleton
cyberphantom52
force-pushed
the
download_manager
branch
from
1dce7d0 to
748106a
Compare
src/downloader.rs
Outdated
There was a problem hiding this comment.
I see the status channel being created but never updated to InProgress, Completed, or Failed (am I wrong?)
There are also more things that are missing but I imagine this is still a WIP.
Good job for the moment <3
There was a problem hiding this comment.
This might not even be working :). I just wanted to put it out so the design can evolve with comments.
src/downloader.rs
Outdated
| progress: progress_tx, | ||
| }; | ||
|
|
||
| let _ = self.queue.try_send(req); |
There was a problem hiding this comment.
Is the try_send safe to be ignored here?
There was a problem hiding this comment.
nope, i just need a new error type to represent the error here.
cyberphantom52
force-pushed
the
download_manager
branch
from
92cb6c6 to
9c29890
Compare
This is to make sure we don't block the runtime.
This is the idomatic way
| #[error("Oneshot: {0}")] | ||
| Oneshot(#[from] tokio::sync::oneshot::error::RecvError), |
There was a problem hiding this comment.
I don't like having to add this specific error type.
src/downloader.rs
Outdated
| pub fn set_max_parallel_downloads(&self, limit: usize) { | ||
| let current = self.semaphore.available_permits(); | ||
| if limit > current { | ||
| self.semaphore.add_permits(limit - current); | ||
| } else if limit < current { | ||
| let to_remove = current - limit; | ||
| for _ in 0..to_remove { | ||
| let _ = self.semaphore.try_acquire(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
On looking carefully, This has a race condition so this needs to be reimplemented.
- added missing impl for GPTK - fixed warnings and clippy
cyberphantom52
force-pushed
the
download_manager
branch
from
cb9d812 to
3fe1e92
Compare
This fixes a bug where if we didn't store the handle to the download somewhere it would automatically cancel the download
src/downloader.rs
Outdated
| } | ||
| } | ||
|
|
||
| async fn dispatcher_thread( |
There was a problem hiding this comment.
dispatch and download threads can outlive the DownloadManager itself. Is this desirable or should we keep track of the threads to manage their lifecycles?
src/downloader/worker.rs
Outdated
| loop { | ||
| tokio::select! { | ||
| _ = req.cancel.cancelled() => { | ||
| drop(file); // Manually drop the file handle to ensure that deletion doesn't fail | ||
| tokio::fs::remove_file(&req.destination).await?; | ||
| return Err(Error::Download(DownloadError::Cancelled)); | ||
| } | ||
| chunk = response.chunk() => { | ||
| match chunk { | ||
| Ok(Some(chunk)) => { | ||
| file.write_all(&chunk).await?; | ||
| bytes_downloaded += chunk.len() as u64; | ||
| update_progress(bytes_downloaded, total_bytes); | ||
| } | ||
| Ok(None) => break, | ||
| Err(e) => { | ||
| drop(file); // Manually drop the file handle to ensure that deletion doesn't fail | ||
| tokio::fs::remove_file(&req.destination).await?; | ||
| return Err(Error::Reqwest(e)) | ||
| }, | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The manual drop, remove_file, req.status.send, req.result.send are error prone, do we have any ideas how to automatically do these
src/downloader/manager.rs
Outdated
| manager | ||
| } | ||
|
|
||
| pub fn download( |
There was a problem hiding this comment.
Should we rename this method to something like new_request_builder since it now returns a download request?
This will help with swapping HTTP backends in the future
cyberphantom52
force-pushed
the
download_manager
branch
from
da3d4b1 to
8354cc9
Compare
Implements #6
TODO: