Skip to content

fix: re-calculate CachedPathImpl::parent and CachedPathImpl::node_modules if the cache is dropped #817

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix: re-calculate CachedPathImpl::parent and CachedPathImpl::node_modules if the cache is dropped #817

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/cache/cache_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ impl<Fs: FileSystem> Cache<Fs> {

path.canonicalizing.store(tid, Ordering::Release);

let res = path.parent().map_or_else(
let res = path.parent(self).map_or_else(
|| Ok(path.normalize_root(self)),
|parent| {
self.canonicalize_impl(&parent).and_then(|parent_canonical| {
Expand All @@ -251,7 +251,7 @@ impl<Fs: FileSystem> Cache<Fs> {
let link = self.fs.read_link(normalized.path())?;
if link.is_absolute() {
return self.canonicalize_impl(&self.value(&link.normalize()));
} else if let Some(dir) = normalized.parent() {
} else if let Some(dir) = normalized.parent(self) {
// Symlink is relative `../../foo.js`, use the path directory
// to resolve this symlink.
return self.canonicalize_impl(&dir.normalize_with(&link, self));
Expand Down
67 changes: 54 additions & 13 deletions src/cache/cached_path.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,13 @@ pub struct CachedPath(pub Arc<CachedPathImpl>);
pub struct CachedPathImpl {
pub hash: u64,
pub path: Box<Path>,
pub parent: Option<Weak<CachedPathImpl>>,
pub parent: Mutex<OptionalWeak<CachedPathImpl>>,
pub is_node_modules: bool,
pub inside_node_modules: bool,
pub meta: OnceLock<Option<FileMetadata>>,
pub canonicalized: Mutex<Result<Weak<CachedPathImpl>, ResolveError>>,
pub canonicalizing: AtomicU64,
pub node_modules: OnceLock<Option<Weak<CachedPathImpl>>>,
pub node_modules: Mutex<OptionalWeak<CachedPathImpl>>,
pub package_json: OnceLock<Option<Arc<PackageJson>>>,
pub tsconfig: OnceLock<Option<Arc<TsConfig>>>,
}
Expand All @@ -45,13 +45,13 @@ impl CachedPathImpl {
Self {
hash,
path,
parent,
parent: Mutex::new(parent.into()),
is_node_modules,
inside_node_modules,
meta: OnceLock::new(),
canonicalized: Mutex::new(Ok(Weak::new())),
canonicalizing: AtomicU64::new(0),
node_modules: OnceLock::new(),
node_modules: Mutex::new(OptionalWeak::new()),
package_json: OnceLock::new(),
tsconfig: OnceLock::new(),
}
Expand All @@ -75,8 +75,16 @@ impl CachedPath {
self.path.to_path_buf()
}

pub(crate) fn parent(&self) -> Option<Self> {
self.0.parent.as_ref().and_then(|weak| weak.upgrade().map(CachedPath))
pub(crate) fn parent<Fs: FileSystem>(&self, cache: &Cache<Fs>) -> Option<Self> {
self.0
.parent
.lock()
.unwrap()
.get_or_init(|| {
let parent_path = self.path.parent()?;
Some(cache.value(parent_path).0)
})
.map(CachedPath)
}

pub(crate) fn is_node_modules(&self) -> bool {
Expand All @@ -103,11 +111,10 @@ impl CachedPath {
ctx: &mut Ctx,
) -> Option<Self> {
self.node_modules
.get_or_init(|| {
self.module_directory("node_modules", cache, ctx).map(|cp| Arc::downgrade(&cp.0))
})
.as_ref()
.and_then(|weak| weak.upgrade().map(CachedPath))
.lock()
.unwrap()
.get_or_init(|| self.module_directory("node_modules", cache, ctx).map(|cp| cp.0))
.map(CachedPath)
}

/// Find package.json of a path by traversing parent directories.
Expand All @@ -124,7 +131,7 @@ impl CachedPath {
let mut cache_value = self.clone();
// Go up directories when the querying path is not a directory
while !cache.is_dir(&cache_value, ctx) {
if let Some(cv) = cache_value.parent() {
if let Some(cv) = cache_value.parent(cache) {
cache_value = cv;
} else {
break;
Expand All @@ -135,7 +142,7 @@ impl CachedPath {
if let Some(package_json) = cache.get_package_json(&cv, options, ctx)? {
return Ok(Some(package_json));
}
cache_value = cv.parent();
cache_value = cv.parent(cache);
}
Ok(None)
}
Expand Down Expand Up @@ -252,3 +259,37 @@ impl fmt::Debug for CachedPath {
f.debug_struct("FsCachedPath").field("path", &self.path).finish()
}
}

// Set (has value): Some(Weak)
// Set (no value): None
// Not set: Some(Weak::new())
#[derive(Debug)]
pub struct OptionalWeak<T>(Option<Weak<T>>);

impl<T> OptionalWeak<T> {
fn new() -> Self {
Self(Some(Weak::new()))
}

fn get_or_init<F: FnOnce() -> Option<Arc<T>>>(&mut self, f: F) -> Option<Arc<T>> {
let weak = self.0.as_ref()?;
if let Some(strong) = weak.upgrade() {
return Some(strong);
}
let value = f();
self.0 = value.as_ref().map(Arc::downgrade);
value
}
}

impl<T> Default for OptionalWeak<T> {
fn default() -> Self {
Self::new()
}
}

impl<T> From<Option<Weak<T>>> for OptionalWeak<T> {
fn from(value: Option<Weak<T>>) -> Self {
Self(value)
}
}
2 changes: 1 addition & 1 deletion src/cache/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ mod hasher;
mod thread_local;

pub use cache_impl::Cache;
pub use cached_path::CachedPath;
pub use cached_path::{CachedPath, OptionalWeak};

#[cfg(test)]
mod tests {
Expand Down
16 changes: 9 additions & 7 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ mod tests;

pub use crate::{
builtins::NODEJS_BUILTINS,
cache::{Cache, CachedPath},
cache::{Cache, CachedPath, OptionalWeak},
error::{JSONError, ResolveError, SpecifierError},
file_system::{FileMetadata, FileSystem, FileSystemOs},
options::{
Expand All @@ -92,7 +92,7 @@ use std::{
borrow::Cow,
cmp::Ordering,
ffi::OsStr,
fmt, iter,
fmt,
path::{Component, Path, PathBuf},
sync::Arc,
};
Expand Down Expand Up @@ -302,7 +302,7 @@ impl<Fs: FileSystem> ResolverGeneric<Fs> {
let inside_node_modules = cached_path.inside_node_modules();
if inside_node_modules {
let mut last = None;
for cp in iter::successors(Some(cached_path.clone()), CachedPath::parent) {
for cp in std::iter::successors(Some(cached_path.clone()), |p| p.parent(&self.cache)) {
if cp.is_node_modules() {
break;
}
Expand Down Expand Up @@ -835,7 +835,8 @@ impl<Fs: FileSystem> ResolverGeneric<Fs> {
// 1. let DIRS = NODE_MODULES_PATHS(START)
// 2. for each DIR in DIRS:
for module_name in &self.options.modules {
for cached_path in std::iter::successors(Some(cached_path.clone()), CachedPath::parent)
for cached_path in
std::iter::successors(Some(cached_path.clone()), |p| p.parent(&self.cache))
{
// Skip if /path/to/node_modules does not exist
if !self.cache.is_dir(&cached_path, ctx) {
Expand Down Expand Up @@ -868,7 +869,7 @@ impl<Fs: FileSystem> ResolverGeneric<Fs> {
// Skip if the directory lead to the scope package does not exist
// i.e. `foo/node_modules/@scope` is not a directory for `foo/node_modules/@scope/package`
if package_name.starts_with('@')
&& let Some(path) = cached_path.parent().as_ref()
&& let Some(path) = cached_path.parent(&self.cache).as_ref()
&& !self.cache.is_dir(path, ctx)
{
continue;
Expand Down Expand Up @@ -1517,7 +1518,7 @@ impl<Fs: FileSystem> ResolverGeneric<Fs> {
})? {
return Ok(Some(Arc::clone(tsconfig)));
}
cache_value = cv.parent();
cache_value = cv.parent(&self.cache);
}
Ok(None)
}
Expand Down Expand Up @@ -1569,7 +1570,8 @@ impl<Fs: FileSystem> ResolverGeneric<Fs> {

// 11. While parentURL is not the file system root,
for module_name in &self.options.modules {
for cached_path in std::iter::successors(Some(cached_path.clone()), CachedPath::parent)
for cached_path in
std::iter::successors(Some(cached_path.clone()), |p| p.parent(&self.cache))
{
// 1. Let packageURL be the URL resolution of "node_modules/" concatenated with packageSpecifier, relative to parentURL.
let Some(cached_path) = self.get_module_directory(&cached_path, module_name, ctx)
Expand Down