From 7a17eba5a748b2362bec48de4edd9f1c214df1f1 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 6 Jun 2024 18:37:41 -0400 Subject: [PATCH] refactor: `NpmRegistryApi` - `#[async_trait(?Send)]` (#24126) --- Cargo.lock | 4 +-- cli/Cargo.toml | 2 +- cli/npm/managed/mod.rs | 45 ++++++++++++++++------------- cli/npm/managed/registry.rs | 2 +- cli/npm/managed/resolvers/common.rs | 2 +- cli/npm/managed/resolvers/global.rs | 2 +- cli/npm/managed/resolvers/local.rs | 11 +++---- 7 files changed, 37 insertions(+), 31 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5c1d7c6e31..6b07f078e8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1734,9 +1734,9 @@ dependencies = [ [[package]] name = "deno_npm" -version = "0.21.2" +version = "0.21.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5c7ae566a3cba78bf05751c20708f28385fe339b1d07bd8daff16316317d4228" +checksum = "a78b95f0daab64d3cfb28b13be2b40d73c8b9563bbce3aa50fc322a7325ce0b9" dependencies = [ "anyhow", "async-trait", diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 548f293e3c..0292b025d5 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -72,7 +72,7 @@ deno_emit = "=0.42.0" deno_graph = { version = "=0.78.0", features = ["tokio_executor"] } deno_lint = { version = "=0.60.0", features = ["docs"] } deno_lockfile.workspace = true -deno_npm = "=0.21.2" +deno_npm = "=0.21.3" deno_runtime = { workspace = true, features = ["include_js_files_for_snapshotting"] } deno_semver = "=0.5.4" deno_task_shell = "=0.16.1" diff --git a/cli/npm/managed/mod.rs b/cli/npm/managed/mod.rs index f139a6f4b0..6a2dfdd673 100644 --- a/cli/npm/managed/mod.rs +++ b/cli/npm/managed/mod.rs @@ -83,26 +83,31 @@ pub async fn create_managed_npm_resolver_for_lsp( ) -> Arc { let npm_cache = create_cache(&options); let npm_api = create_api(&options, npm_cache.clone()); - let snapshot = match resolve_snapshot(&npm_api, options.snapshot).await { - Ok(snapshot) => snapshot, - Err(err) => { - log::warn!("failed to resolve snapshot: {}", err); - None - } - }; - create_inner( - options.fs, - options.http_client_provider, - options.maybe_lockfile, - npm_api, - npm_cache, - options.npmrc, - options.package_json_installer, - options.text_only_progress_bar, - options.maybe_node_modules_path, - options.npm_system_info, - snapshot, - ) + // spawn due to the lsp's `Send` requirement + deno_core::unsync::spawn(async move { + let snapshot = match resolve_snapshot(&npm_api, options.snapshot).await { + Ok(snapshot) => snapshot, + Err(err) => { + log::warn!("failed to resolve snapshot: {}", err); + None + } + }; + create_inner( + options.fs, + options.http_client_provider, + options.maybe_lockfile, + npm_api, + npm_cache, + options.npmrc, + options.package_json_installer, + options.text_only_progress_bar, + options.maybe_node_modules_path, + options.npm_system_info, + snapshot, + ) + }) + .await + .unwrap() } pub async fn create_managed_npm_resolver( diff --git a/cli/npm/managed/registry.rs b/cli/npm/managed/registry.rs index 14c3bd38f3..8f15d619b9 100644 --- a/cli/npm/managed/registry.rs +++ b/cli/npm/managed/registry.rs @@ -50,7 +50,7 @@ impl CliNpmRegistryApi { } } -#[async_trait] +#[async_trait(?Send)] impl NpmRegistryApi for CliNpmRegistryApi { async fn package_info( &self, diff --git a/cli/npm/managed/resolvers/common.rs b/cli/npm/managed/resolvers/common.rs index 4cdad1f996..5003e6a058 100644 --- a/cli/npm/managed/resolvers/common.rs +++ b/cli/npm/managed/resolvers/common.rs @@ -24,7 +24,7 @@ use deno_runtime::deno_node::NodeResolutionMode; use crate::npm::managed::cache::TarballCache; /// Part of the resolution that interacts with the file system. -#[async_trait] +#[async_trait(?Send)] pub trait NpmPackageFsResolver: Send + Sync { /// Specifier for the root directory. fn root_dir_url(&self) -> &Url; diff --git a/cli/npm/managed/resolvers/global.rs b/cli/npm/managed/resolvers/global.rs index a6a071e077..05c2472cf3 100644 --- a/cli/npm/managed/resolvers/global.rs +++ b/cli/npm/managed/resolvers/global.rs @@ -70,7 +70,7 @@ impl GlobalNpmPackageResolver { } } -#[async_trait] +#[async_trait(?Send)] impl NpmPackageFsResolver for GlobalNpmPackageResolver { fn root_dir_url(&self) -> &Url { self.cache.root_dir_url() diff --git a/cli/npm/managed/resolvers/local.rs b/cli/npm/managed/resolvers/local.rs index 1de8f40667..c64aacc397 100644 --- a/cli/npm/managed/resolvers/local.rs +++ b/cli/npm/managed/resolvers/local.rs @@ -5,12 +5,14 @@ mod bin_entries; use std::borrow::Cow; +use std::cell::RefCell; use std::cmp::Ordering; use std::collections::HashMap; use std::collections::HashSet; use std::fs; use std::path::Path; use std::path::PathBuf; +use std::rc::Rc; use std::sync::Arc; use crate::cache::CACHE_PERM; @@ -29,7 +31,6 @@ use deno_core::anyhow::Context; use deno_core::error::AnyError; use deno_core::futures::stream::FuturesUnordered; use deno_core::futures::StreamExt; -use deno_core::parking_lot::Mutex; use deno_core::url::Url; use deno_npm::resolution::NpmResolutionSnapshot; use deno_npm::NpmPackageCacheFolderId; @@ -141,7 +142,7 @@ impl LocalNpmPackageResolver { } } -#[async_trait] +#[async_trait(?Send)] impl NpmPackageFsResolver for LocalNpmPackageResolver { fn root_dir_url(&self) -> &Url { &self.root_node_modules_url @@ -296,7 +297,7 @@ async fn sync_resolution_with_fs( let mut cache_futures = FuturesUnordered::new(); let mut newest_packages_by_name: HashMap<&String, &NpmResolutionPackage> = HashMap::with_capacity(package_partitions.packages.len()); - let bin_entries = Arc::new(Mutex::new(bin_entries::BinEntries::new())); + let bin_entries = Rc::new(RefCell::new(bin_entries::BinEntries::new())); for package in &package_partitions.packages { if let Some(current_pkg) = newest_packages_by_name.get_mut(&package.id.nv.name) @@ -349,7 +350,7 @@ async fn sync_resolution_with_fs( if package.bin.is_some() { bin_entries_to_setup - .lock() + .borrow_mut() .add(package.clone(), package_path); } @@ -482,7 +483,7 @@ async fn sync_resolution_with_fs( // 6. Set up `node_modules/.bin` entries for packages that need it. { - let bin_entries = std::mem::take(&mut *bin_entries.lock()); + let bin_entries = std::mem::take(&mut *bin_entries.borrow_mut()); bin_entries.finish(snapshot, &bin_node_modules_dir_path)?; }