From 896a492553fc2409de945a52729f8b9996ae48e5 Mon Sep 17 00:00:00 2001 From: khai96_ Date: Wed, 20 Sep 2023 09:38:51 +0700 Subject: [PATCH 1/5] refactor: explicit `drop` --- crates/tarball/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/tarball/src/lib.rs b/crates/tarball/src/lib.rs index 3219de225..27d868280 100644 --- a/crates/tarball/src/lib.rs +++ b/crates/tarball/src/lib.rs @@ -128,6 +128,7 @@ pub async fn download_tarball_to_store( } CacheValue::InProgress(notify) => Arc::clone(notify), }; + drop(cache_lock); notify.notified().await; if let Some(cached) = cache.get(package_url) { if let CacheValue::Available(cas_paths) = &*cached.read().await { From 4250f400d4520db8a6416445f7fecec93aff0ec3 Mon Sep 17 00:00:00 2001 From: khai96_ Date: Wed, 20 Sep 2023 09:43:40 +0700 Subject: [PATCH 2/5] feat: programmer error should be a panic --- crates/tarball/src/lib.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/crates/tarball/src/lib.rs b/crates/tarball/src/lib.rs index 27d868280..a3d328ab0 100644 --- a/crates/tarball/src/lib.rs +++ b/crates/tarball/src/lib.rs @@ -135,10 +135,7 @@ pub async fn download_tarball_to_store( return Ok(Arc::clone(cas_paths)); } } - Err(TarballError::Io(std::io::Error::new( - std::io::ErrorKind::Other, - "Failed to get or compute tarball data", - ))) + panic!("Failed to get or compute tarball data for {package_url:?}"); } else { let notify = Arc::new(Notify::new()); let cache_lock = notify From 217ccabdf0c71174579929e9b30e6f8b08add5ef Mon Sep 17 00:00:00 2001 From: khai96_ Date: Wed, 20 Sep 2023 09:47:51 +0700 Subject: [PATCH 3/5] feat: more diagnostic --- crates/tarball/src/lib.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/tarball/src/lib.rs b/crates/tarball/src/lib.rs index a3d328ab0..d2727500a 100644 --- a/crates/tarball/src/lib.rs +++ b/crates/tarball/src/lib.rs @@ -128,6 +128,8 @@ pub async fn download_tarball_to_store( } CacheValue::InProgress(notify) => Arc::clone(notify), }; + + tracing::info!(target: "pacquet::download", ?package_url, "Wait for cache"); drop(cache_lock); notify.notified().await; if let Some(cached) = cache.get(package_url) { @@ -143,7 +145,9 @@ pub async fn download_tarball_to_store( .pipe(CacheValue::InProgress) .pipe(RwLock::new) .pipe(Arc::new); - cache.insert(package_url.to_string(), Arc::clone(&cache_lock)); + if cache.insert(package_url.to_string(), Arc::clone(&cache_lock)).is_some() { + tracing::warn!(target: "pacquet::download", ?package_url, "Race condition detected when writing to cache"); + } let cas_paths = download_tarball_to_store_uncached( package_url, http_client, From 0b4896b3cd515eb411037546fc9f3b965ea30794 Mon Sep 17 00:00:00 2001 From: khai96_ Date: Wed, 20 Sep 2023 09:50:41 +0700 Subject: [PATCH 4/5] refactor: remove `drop`, reuse `cache_lock` --- crates/tarball/src/lib.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/crates/tarball/src/lib.rs b/crates/tarball/src/lib.rs index d2727500a..e254fe2c5 100644 --- a/crates/tarball/src/lib.rs +++ b/crates/tarball/src/lib.rs @@ -130,12 +130,9 @@ pub async fn download_tarball_to_store( }; tracing::info!(target: "pacquet::download", ?package_url, "Wait for cache"); - drop(cache_lock); notify.notified().await; - if let Some(cached) = cache.get(package_url) { - if let CacheValue::Available(cas_paths) = &*cached.read().await { - return Ok(Arc::clone(cas_paths)); - } + if let CacheValue::Available(cas_paths) = &*cache_lock.read().await { + return Ok(Arc::clone(cas_paths)); } panic!("Failed to get or compute tarball data for {package_url:?}"); } else { From a6cade27b8e7597d34d7b96c680bf75db7d879f4 Mon Sep 17 00:00:00 2001 From: khai96_ Date: Wed, 20 Sep 2023 09:56:11 +0700 Subject: [PATCH 5/5] docs: mark as unreachable --- crates/tarball/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/tarball/src/lib.rs b/crates/tarball/src/lib.rs index e254fe2c5..3908006ea 100644 --- a/crates/tarball/src/lib.rs +++ b/crates/tarball/src/lib.rs @@ -134,7 +134,7 @@ pub async fn download_tarball_to_store( if let CacheValue::Available(cas_paths) = &*cache_lock.read().await { return Ok(Arc::clone(cas_paths)); } - panic!("Failed to get or compute tarball data for {package_url:?}"); + unreachable!("Failed to get or compute tarball data for {package_url:?}"); } else { let notify = Arc::new(Notify::new()); let cache_lock = notify