refactor(npm): improve locking around updating npm resolution (denola…

…nd#24104)

Introduces a `SyncReadAsyncWriteLock` to make it harder to write to the
npm resolution without first waiting async in a queue. For the npm
resolution, reading synchronously is fine, but when updating, someone
should wait async, clone the data, then write the data at the end back.

cli/npm/managed/cache/mod.rs

@@ -28,7 +28,6 @@ use crate::util::fs::hard_link_dir_recursive;
 mod registry_info;
 mod tarball;
 mod tarball_extract;
-mod value_creator;
 
 pub use registry_info::RegistryInfoDownloader;
 pub use tarball::TarballCache;

cli/npm/managed/cache/registry_info.rs

@@ -13,24 +13,33 @@ use deno_core::futures::FutureExt;
 use deno_core::parking_lot::Mutex;
 use deno_core::serde_json;
 use deno_core::url::Url;
-use deno_npm::npm_rc::RegistryConfig;
 use deno_npm::npm_rc::ResolvedNpmRc;
 use deno_npm::registry::NpmPackageInfo;
 
 use crate::args::CacheSetting;
 use crate::http_util::HttpClientProvider;
 use crate::npm::common::maybe_auth_header_for_npm_registry;
 use crate::util::progress_bar::ProgressBar;
+use crate::util::sync::MultiRuntimeAsyncValueCreator;
 
-use super::value_creator::MultiRuntimeAsyncValueCreator;
 use super::NpmCache;
 
 // todo(dsherret): create seams and unit test this
 
+type LoadResult = Result<FutureResult, Arc<AnyError>>;
+type LoadFuture = LocalBoxFuture<'static, LoadResult>;
+
+#[derive(Debug, Clone)]
+enum FutureResult {
+ PackageNotExists,
+ SavedFsCache(Arc<NpmPackageInfo>),
+ ErroredFsCache(Arc<NpmPackageInfo>),
+}
+
 #[derive(Debug, Clone)]
 enum MemoryCacheItem {
  /// The cache item hasn't loaded yet.
- Pending(Arc<MultiRuntimeAsyncValueCreator<FutureResult>>),
+ Pending(Arc<MultiRuntimeAsyncValueCreator<LoadResult>>),
  /// The item has loaded in the past and was stored in the file system cache.
  /// There is no reason to request this package from the npm registry again
  /// for the duration of execution.
@@ -40,16 +49,6 @@ enum MemoryCacheItem {
  MemoryCached(Result<Option<Arc<NpmPackageInfo>>, Arc<AnyError>>),
 }
 
-#[derive(Debug, Clone)]
-enum FutureResult {
- PackageNotExists,
- SavedFsCache(Arc<NpmPackageInfo>),
- ErroredFsCache(Arc<NpmPackageInfo>),
-}
-
-type PendingRegistryLoadFuture =
- LocalBoxFuture<'static, Result<FutureResult, AnyError>>;
-
 /// Downloads packuments from the npm registry.
 ///
 /// This is shared amongst all the workers.
@@ -82,26 +81,18 @@ impl RegistryInfoDownloader {
  self: &Arc<Self>,
  name: &str,
  ) -> Result<Option<Arc<NpmPackageInfo>>, AnyError> {
- let registry_url = self.npmrc.get_registry_url(name);
- let registry_config = self.npmrc.get_registry_config(name);
-
- self
- .load_package_info_inner(name, registry_url, registry_config)
- .await
- .with_context(|| {
- format!(
- "Error getting response at {} for package \"{}\"",
- self.get_package_url(name, registry_url),
- name
- )
- })
+ self.load_package_info_inner(name).await.with_context(|| {
+ format!(
+ "Error getting response at {} for package \"{}\"",
+ self.get_package_url(name),
+ name
+ )
+ })
  }
 
  async fn load_package_info_inner(
  self: &Arc<Self>,
  name: &str,
- registry_url: &Url,
- registry_config: &RegistryConfig,
  ) -> Result<Option<Arc<NpmPackageInfo>>, AnyError> {
  if *self.cache.cache_setting() == CacheSetting::Only {
  return Err(custom_error(
@@ -117,9 +108,11 @@ impl RegistryInfoDownloader {
  if let Some(cache_item) = mem_cache.get(name) {
  cache_item.clone()
  } else {
- let future =
- self.create_load_future(name, registry_url, registry_config);
- let value_creator = MultiRuntimeAsyncValueCreator::new(future);
+ let value_creator = MultiRuntimeAsyncValueCreator::new({
+ let downloader = self.clone();
+ let name = name.to_string();
+ Box::new(move || downloader.create_load_future(&name))
+ });
  let cache_item = MemoryCacheItem::Pending(Arc::new(value_creator));
  mem_cache.insert(name.to_string(), cache_item.clone());
  cache_item
@@ -138,11 +131,7 @@ impl RegistryInfoDownloader {
  maybe_info.clone().map_err(|e| anyhow!("{}", e))
  }
  MemoryCacheItem::Pending(value_creator) => {
- let downloader = self.clone();
- let future = value_creator.get(move || {
- downloader.create_load_future(name, registry_url, registry_config)
- });
- match future.await {
+ match value_creator.get().await {
  Ok(FutureResult::SavedFsCache(info)) => {
  // return back the future and mark this package as having
  // been saved in the cache for next time it's requested
@@ -199,14 +188,10 @@ impl RegistryInfoDownloader {
  }
  }
 
- fn create_load_future(
- self: &Arc<Self>,
- name: &str,
- registry_url: &Url,
- registry_config: &RegistryConfig,
- ) -> PendingRegistryLoadFuture {
+ fn create_load_future(self: &Arc<Self>, name: &str) -> LoadFuture {
  let downloader = self.clone();
- let package_url = self.get_package_url(name, registry_url);
+ let package_url = self.get_package_url(name);
+ let registry_config = self.npmrc.get_registry_config(name);
  let maybe_auth_header = maybe_auth_header_for_npm_registry(registry_config);
  let guard = self.progress_bar.update(package_url.as_str());
  let name = name.to_string();
@@ -242,10 +227,12 @@ impl RegistryInfoDownloader {
  None => Ok(FutureResult::PackageNotExists),
  }
  }
+ .map(|r| r.map_err(Arc::new))
  .boxed_local()
  }
 
- fn get_package_url(&self, name: &str, registry_url: &Url) -> Url {
+ fn get_package_url(&self, name: &str) -> Url {
+ let registry_url = self.npmrc.get_registry_url(name);
  // list of all characters used in npm packages:
  // !, ', (, ), *, -, ., /, [0-9], @, [A-Za-z], _, ~
  const ASCII_SET: percent_encoding::AsciiSet =

cli/npm/managed/cache/tarball.rs

@@ -20,18 +20,21 @@ use crate::args::CacheSetting;
 use crate::http_util::HttpClientProvider;
 use crate::npm::common::maybe_auth_header_for_npm_registry;
 use crate::util::progress_bar::ProgressBar;
+use crate::util::sync::MultiRuntimeAsyncValueCreator;
 
 use super::tarball_extract::verify_and_extract_tarball;
 use super::tarball_extract::TarballExtractionMode;
-use super::value_creator::MultiRuntimeAsyncValueCreator;
 use super::NpmCache;
 
 // todo(dsherret): create seams and unit test this
 
+type LoadResult = Result<(), Arc<AnyError>>;
+type LoadFuture = LocalBoxFuture<'static, LoadResult>;
+
 #[derive(Debug, Clone)]
 enum MemoryCacheItem {
  /// The cache item hasn't finished yet.
- Pending(Arc<MultiRuntimeAsyncValueCreator<()>>),
+ Pending(Arc<MultiRuntimeAsyncValueCreator<LoadResult>>),
  /// The result errored.
  Errored(Arc<AnyError>),
  /// This package has already been cached.
@@ -91,8 +94,14 @@ impl TarballCache {
  if let Some(cache_item) = mem_cache.get(package_nv) {
  cache_item.clone()
  } else {
- let future = self.create_setup_future(package_nv.clone(), dist.clone());
- let value_creator = MultiRuntimeAsyncValueCreator::new(future);
+ let value_creator = MultiRuntimeAsyncValueCreator::new({
+ let tarball_cache = self.clone();
+ let package_nv = package_nv.clone();
+ let dist = dist.clone();
+ Box::new(move || {
+ tarball_cache.create_setup_future(package_nv.clone(), dist.clone())
+ })
+ });
  let cache_item = MemoryCacheItem::Pending(Arc::new(value_creator));
  mem_cache.insert(package_nv.clone(), cache_item.clone());
  cache_item
@@ -103,12 +112,7 @@ impl TarballCache {
  MemoryCacheItem::Cached => Ok(()),
  MemoryCacheItem::Errored(err) => Err(anyhow!("{}", err)),
  MemoryCacheItem::Pending(creator) => {
- let tarball_cache = self.clone();
- let result = creator
- .get(move || {
- tarball_cache.create_setup_future(package_nv.clone(), dist.clone())
- })
- .await;
+ let result = creator.get().await;
  match result {
  Ok(_) => {
  *self.memory_cache.lock().get_mut(package_nv).unwrap() =
@@ -130,7 +134,7 @@ impl TarballCache {
  self: &Arc<Self>,
  package_nv: PackageNv,
  dist: NpmPackageVersionDistInfo,
- ) -> LocalBoxFuture<'static, Result<(), AnyError>> {
+ ) -> LoadFuture {
  let tarball_cache = self.clone();
  async move {
  let registry_url = tarball_cache.npmrc.get_registry_url(&package_nv.name);
@@ -197,6 +201,8 @@ impl TarballCache {
  bail!("Could not find npm package tarball at: {}", dist.tarball);
  }
  }
- }.boxed_local()
+ }
+ .map(|r| r.map_err(Arc::new))
+ .boxed_local()
  }
 }