diff --git a/CODE_STYLE_GUIDE.md b/CODE_STYLE_GUIDE.md index 052c42a8e..5108546eb 100644 --- a/CODE_STYLE_GUIDE.md +++ b/CODE_STYLE_GUIDE.md @@ -174,3 +174,57 @@ assert_eq!(received, 5); ``` If the assertion fails, the value of `received` will appear alongside the error message. + +### Cloning an atomic counter + +Prefer using `Arc::clone` or `Rc::clone` to vague `.clone()` or `Clone::clone`. + +**Error resistance:** Explicitly specifying the cloned type would avoid accidentally cloning the wrong type. As seen below: + +```rust +fn my_function(value: Arc>) { + // ... do many things here + let value_clone = value.clone(); // inexpensive clone + tokio::task::spawn(async { + // ... do stuff with value_clone + }) +} +``` + +The above function could easily refactored into the following code: + +```rust +fn my_function(value: &Vec) { + // ... do many things here + let value_clone = value.clone(); // expensive clone, oops + tokio::task::spawn(async { + // ... do stuff with value_clone + }) +} +``` + +With an explicit `Arc::clone`, however, the performance characteristic will never be missed: + +```rust +fn my_function(value: Arc>) { + // ... do many things here + let value_clone = Arc::clone(&value); // no compile error + tokio::task::spawn(async { + // ... do stuff with value_clone + }) +} +``` + +```rust +fn my_function(value: &Vec) { + // ... do many things here + let value_clone = Arc::clone(&value); // compile error + tokio::task::spawn(async { + // ... do stuff with value_clone + }) +} +``` + +The above code is still valid code, and the Rust compiler doesn't error, but it has a different performance characteristic now. + +**Readability:** The generic `.clone()` or `Clone::clone` often implies an expensive operation (for example: cloning a `Vec`), but `Arc` and `Rc` are not as expensive as the generic `.clone()`. Explicitly mark the cloned type would aid in future refactoring. diff --git a/crates/tarball/src/lib.rs b/crates/tarball/src/lib.rs index c81755abc..3219de225 100644 --- a/crates/tarball/src/lib.rs +++ b/crates/tarball/src/lib.rs @@ -122,22 +122,16 @@ pub async fn download_tarball_to_store( package_url: &str, ) -> Result>, TarballError> { if let Some(cache_lock) = cache.get(package_url) { - let notify; - { - let cache_value = cache_lock.write().await; - match &*cache_value { - CacheValue::Available(cas_paths) => { - return Ok(cas_paths.clone()); - } - CacheValue::InProgress(existing_notify) => { - notify = existing_notify.clone(); - } + let notify = match &*cache_lock.write().await { + CacheValue::Available(cas_paths) => { + return Ok(Arc::clone(cas_paths)); } - } + CacheValue::InProgress(notify) => Arc::clone(notify), + }; notify.notified().await; if let Some(cached) = cache.get(package_url) { if let CacheValue::Available(cas_paths) = &*cached.read().await { - return Ok(cas_paths.clone()); + return Ok(Arc::clone(cas_paths)); } } Err(TarballError::Io(std::io::Error::new( @@ -146,8 +140,12 @@ pub async fn download_tarball_to_store( ))) } else { let notify = Arc::new(Notify::new()); - let cache_lock = Arc::new(RwLock::new(CacheValue::InProgress(notify.clone()))); - cache.insert(package_url.to_string(), cache_lock.clone()); + let cache_lock = notify + .pipe_ref(Arc::clone) + .pipe(CacheValue::InProgress) + .pipe(RwLock::new) + .pipe(Arc::new); + cache.insert(package_url.to_string(), Arc::clone(&cache_lock)); let cas_paths = download_tarball_to_store_uncached( package_url, http_client, @@ -157,7 +155,7 @@ pub async fn download_tarball_to_store( ) .await?; let mut cache_write = cache_lock.write().await; - *cache_write = CacheValue::Available(cas_paths.clone()); + *cache_write = CacheValue::Available(Arc::clone(&cas_paths)); notify.notify_waiters(); Ok(cas_paths) }