refactor: explicit Arc::clone (#125)

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<Vec<u8>>) {
+    // ... 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<u8>) {
+    // ... 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<Vec<u8>>) {
+    // ... 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<u8>) {
+    // ... 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.

crates/tarball/src/lib.rs

@@ -122,22 +122,16 @@ pub async fn download_tarball_to_store(
     package_url: &str,
 ) -> Result<Arc<HashMap<OsString, PathBuf>>, 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)
     }