evan-sm / cyberdrop-dl

📦🌏 Cyberdrop.me album downloader
https://crates.io/crates/cyberdrop-dl
138 stars 15 forks source link

Semaphore is immediately released after acquiring it #1

Closed pheki closed 2 years ago

pheki commented 2 years ago

Hey!

There are two instances of the following line in the code:

let _ = sem_clone.acquire().await.unwrap();

This will acquire the semaphore permit and immediately drop it, as let _ does not actually bind the value (it's ignored). This means that may be more concurrent downloads happening than planned.

A quick fix would be let _permit = sem_clone.acquire().await.unwrap();, then it'd be dropped at the end of the scope. Note that you may need to increment the batch size to keep the current download speed.

Also, I personally find it better to name ignored variables, such as let _result = download_album [..] (line 61) and let _count = io::copy [...] (line 127), making it easier (when reading) to know what's being ignored.

References on _ not binding:

The Rust Programming Language: Ignoring an Entire Value with _ https://github.com/rust-lang/rust/issues/40096

evan-sm commented 2 years ago

Gracias! Fixed!