Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
39 changes: 30 additions & 9 deletions src/bundler/transpiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -766,16 +766,26 @@ impl<'a> Transpiler<'a> {
merge_tsconfig_jsx_into(tsconfig, &mut self.options.jsx);
}

let Some(dir) = dir_info.get_entries(self.resolver.generation) else {
// Refresh the listing at our generation (takes and releases
// `entries_mutex`), then copy the basenames out under the
// lock: `dot_env::Loader::load` probes the listing between
// file reads, and another resolver at a newer generation
// rewrites the `DirEntry` map in place under `entries_mutex`.
Comment thread
robobun marked this conversation as resolved.
Outdated
if dir_info.get_entries(self.resolver.generation).is_none() {
return Ok(());
}
let dir = {
let _entries_lock = bun_resolver::fs::FileSystem::instance()
.fs
.entries_mutex
.lock_guard();
match dir_info.get_entries_const() {
Some(entries) => DotEnvProbeKeys(
entries.data.iter().map(|(k, _)| Box::from(&**k)).collect(),
),
None => return Ok(()),
}
};
// `get_entries` returns `*mut bun_resolver::fs::DirEntry`
// (BSSMap-owned). `dot_env::Loader::load` takes
// `impl DirEntryProbe` (bun_dotenv sits below `bun_resolver`
// in the crate graph); `bun_resolver::fs::DirEntry` impls it.
// SAFETY: BSSMap singleton owns `*dir`; single-threaded path —
// sole `&mut` for the call.
let dir: &mut bun_resolver::fs::DirEntry = unsafe { &mut *dir };

// `Env.files: Box<[Box<[u8]>]>` but `Loader::load`
// wants `&[&[u8]]`. Re-borrow into a small Vec; the explicit
Expand All @@ -789,7 +799,7 @@ impl<'a> Transpiler<'a> {
} else {
dot_env::DotEnvFileSuffix::Development
};
env.load(dir, &env_files, suffix, skip_default_env)?;
env.load(&dir, &env_files, suffix, skip_default_env)?;
}
DotEnvBehavior::disable => {
env.load_process()?;
Expand All @@ -809,6 +819,17 @@ impl<'a> Transpiler<'a> {
}
}

/// Basenames copied out of a cached `DirEntry` under `entries_mutex` so
/// `dot_env::Loader::load` can probe them without holding the lock across its
/// file reads (see `run_env_loader`).
Comment thread
robobun marked this conversation as resolved.
Outdated
struct DotEnvProbeKeys(Vec<Box<[u8]>>);

impl dot_env::DirEntryProbe for DotEnvProbeKeys {
fn has_comptime_query(&self, query_lower: &'static [u8]) -> bool {
self.0.iter().any(|k| **k == *query_lower)
}
}

// ══════════════════════════════════════════════════════════════════════════
// `ParseResult` / `AlreadyBundled` / `ParseOptions` + `Transpiler::parse*`
// — used by `ModuleLoader::transpile_source_code` (jsc_hooks.rs) and
Expand Down
5 changes: 4 additions & 1 deletion src/jsc/hot_reloader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1180,7 +1180,10 @@ where
// the per-entry mutex.
let _entry_guard = ent.mutex.lock_guard();
ent.set_cache_fd(Fd::INVALID);
ent.need_stat.set(true);
ent.need_stat.store(
true,
core::sync::atomic::Ordering::Release,
);
}
path_string = ent.abs_path;
file_hash = Watcher::get_hash(path_string.as_bytes());
Expand Down
51 changes: 33 additions & 18 deletions src/resolver/dir_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -238,34 +238,49 @@ impl DirInfo {
}
}

/// Shared-borrow variant of [`get_entries`](Self::get_entries) for the
/// read-only call sites (`.get()`, `.fd`, iteration). The `DirEntry` is a
/// slot in the BSSMap-backed `EntriesOptionMap` singleton (ARENA — process
/// lifetime), so a `&'static` reborrow of the `&'static mut` returned by
/// `entries_at` is sound and needs no `unsafe` here. Prefer this over
/// `get_entries` + per-site raw deref whenever the caller only reads.
pub(crate) fn get_entries_ref(&self, generation: Generation) -> Option<&'static fs::DirEntry> {
/// Shared-borrow variant of [`get_entries`](Self::get_entries) for call
/// sites that already hold `entries_mutex` (the mutex is non-recursive);
/// see [`RealFS::entries_at_locked`](fs::RealFS::entries_at_locked). The
/// `DirEntry` is a slot in the BSSMap-backed `EntriesOptionMap` singleton
/// (ARENA — process lifetime), so a `&'static` reborrow of the
/// `&'static mut` returned by `entries_at_locked` is sound and needs no
/// `unsafe` here. The `.data` map must only be probed/iterated while the
/// lock is held; use [`get_entry`](Self::get_entry) for one-shot lookups.
Comment thread
robobun marked this conversation as resolved.
Outdated
Comment thread
robobun marked this conversation as resolved.
pub(crate) fn get_entries_ref_locked(
&self,
generation: Generation,
) -> Option<&'static fs::DirEntry> {
let entries_ptr = fs::FileSystem::instance()
.fs
.entries_at(self.entries, generation)?;
.entries_at_locked(self.entries, generation)?;
match entries_ptr {
fs::EntriesOption::Entries(entries) => Some(&**entries),
fs::EntriesOption::Err(_) => None,
}
}

/// [`get_entries_ref`](Self::get_entries_ref) for call sites that already
/// hold `entries_mutex` (the mutex is non-recursive); see
/// [`RealFS::entries_at_locked`](fs::RealFS::entries_at_locked).
pub(crate) fn get_entries_ref_locked(
/// Generation-checked lookup of one basename in this directory's cached
/// listing, performed in a single `entries_mutex` critical section. A
/// concurrent resolver with a newer generation rewrites the `DirEntry` in
/// place under that lock ([`RealFS::entries_at_locked`](fs::RealFS::entries_at_locked)
/// drops the old `data` map's buckets), so probing the map after the lock
/// is released can walk freed buckets. The returned lookup wraps a raw
/// pointer into the process-lifetime `EntryStore`, which stays valid after
/// the lock is dropped.
Comment thread
robobun marked this conversation as resolved.
pub(crate) fn get_entry(
&self,
generation: Generation,
) -> Option<&'static fs::DirEntry> {
let entries_ptr = fs::FileSystem::instance()
.fs
.entries_at_locked(self.entries, generation)?;
match entries_ptr {
fs::EntriesOption::Entries(entries) => Some(&**entries),
query: &[u8],
) -> Option<fs::EntryLookup<'static>> {
let rfs = &mut fs::FileSystem::instance().fs;
// `MutexGuard` stores the mutex by raw pointer, so holding it does not
// keep `rfs` borrowed (same pattern as `RealFS::entries_at`).
Comment thread
robobun marked this conversation as resolved.
Outdated
let _lock = rfs.entries_mutex.lock_guard();
match rfs.entries_at_locked(self.entries, generation)? {
fs::EntriesOption::Entries(entries) => {
let entries: &'static fs::DirEntry = entries;
entries.get(query)
}
fs::EntriesOption::Err(_) => None,
}
}
Expand Down
48 changes: 34 additions & 14 deletions src/resolver/fs.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use core::sync::atomic::{AtomicBool, Ordering};
use std::borrow::Cow;
use std::io::Write as _;

Expand Down Expand Up @@ -136,8 +137,11 @@
// `Entry::symlink` while callers hold a shared
// `&Entry`. `EntryCache` is `Copy`, so `Cell` gives us safe
// `.get()/.set()` through `&self` — the per-entry `mutex` serializes every
// rewrite of these `Cell`s across threads (the `unsafe impl Sync for Entry`
// below opts back in under that external-locking discipline).
// rewrite of these fields across threads (the `unsafe impl Sync for Entry`
// below opts back in under that external-locking discipline). `need_stat`
// is atomic because the `kind()`/`symlink()` fast path reads it without the
// mutex: the Release store after the `cache` write paired with the Acquire
// load is what publishes `cache` to those lock-free readers.
Comment thread
robobun marked this conversation as resolved.
pub struct Entry {
pub(crate) cache: core::cell::Cell<EntryCache>,
pub dir: &'static [u8],
Expand All @@ -148,7 +152,7 @@
pub(crate) base_lowercase_: strings::StringOrTinyString,

pub mutex: Mutex,
pub need_stat: core::cell::Cell<bool>,
pub need_stat: AtomicBool,

pub abs_path: Interned,
}
Expand Down Expand Up @@ -226,10 +230,10 @@
// Generic over `R: EntryKindResolver` so this block is independent of
// which `RealFS` copy `fs` points at (see file-top comment).
pub unsafe fn kind<R: EntryKindResolver>(&self, fs: *mut R, store_fd: bool) -> EntryKind {
if self.need_stat.get() {
if self.need_stat.load(Ordering::Acquire) {
let _guard = self.mutex.lock_guard();
if self.need_stat.get() {
self.need_stat.set(false);
// Relaxed: every write happens under `mutex`, which we hold.
if self.need_stat.load(Ordering::Relaxed) {
// This is technically incorrect, but we are choosing not to handle errors here
// SAFETY: `fs` points at the process-global RealFS singleton; `resolve_kind`
// only does syscalls + string interning, so the short `&mut` cannot alias.
Expand All @@ -240,8 +244,15 @@
store_fd,
) {
Ok(c) => self.cache.set(c),
Err(_) => return self.cache().kind,
Err(_) => {
self.need_stat.store(false, Ordering::Release);
return self.cache().kind;
}
}
// Clear the flag only after the `cache` write: lock-free readers
// that observe `false` skip the mutex, so this Release store is
// what publishes `cache` to them.
Comment thread
robobun marked this conversation as resolved.
self.need_stat.store(false, Ordering::Release);
}
}
self.cache().kind
Expand All @@ -256,10 +267,10 @@
fs: *mut R,
store_fd: bool,
) -> &'static [u8] {
if self.need_stat.get() {
if self.need_stat.load(Ordering::Acquire) {
let _guard = self.mutex.lock_guard();
if self.need_stat.get() {
self.need_stat.set(false);
// Relaxed: every write happens under `mutex`, which we hold.
if self.need_stat.load(Ordering::Relaxed) {
// This error can happen if the file was deleted between the time the directory
// was scanned and the time it was read
// SAFETY: see the note on `Entry::kind`.
Expand All @@ -270,8 +281,13 @@
store_fd,
) {
Ok(c) => self.cache.set(c),
Err(_) => return b"",
Err(_) => {
self.need_stat.store(false, Ordering::Release);
return b"";
}
}
// See `Entry::kind`: Release-publish `cache` before the flag clear.
self.need_stat.store(false, Ordering::Release);
}
}
self.cache().symlink.as_bytes()
Expand Down Expand Up @@ -479,13 +495,17 @@
let _guard = existing.mutex.lock_guard();
existing.dir = self.dir;

existing.need_stat.set(
existing.need_stat.get()
// Relaxed load: writes are serialized on the per-entry
// mutex held above; Release store pairs with the Acquire
// fast-path load in `kind()`/`symlink()`.
existing.need_stat.store(
existing.need_stat.load(Ordering::Relaxed)
|| found_kind.is_none()
|| Some(existing.cache().kind) != found_kind,
Ordering::Release,
);
// TODO: is this right?
if Some(existing.cache().kind) != found_kind {

Check failure on line 508 in src/resolver/fs.rs

View check run for this annotation

Claude / Claude Code Review

Recycle path writes cache AFTER need_stat store — same torn-read window PR claims to close

The recycle path Release-stores `need_stat = true` **before** calling `set_cache_kind`/`set_cache_symlink` — a concurrent `kind()`/`symlink()` reader that Acquire-loads the old `false` skips the per-entry mutex and does a non-atomic `Cell::get()` while this thread is inside `Cell::set()` on the ~24-byte cache, tearing the 16-byte `Interned` symlink exactly like the `0x25` fault this PR describes. Reordering wouldn't help (a false→true store can never publish anything to a reader that loaded the
Comment thread
robobun marked this conversation as resolved.
Outdated
// if found_kind is null, we have set need_stat above, so we
// store an arbitrary kind
existing.set_cache_kind(found_kind.unwrap_or(EntryKind::File));
Expand Down Expand Up @@ -536,7 +556,7 @@
// Call "stat" lazily for performance. The "@material-ui/icons" package
// contains a directory with over 11,000 entries in it and running "stat"
// for each entry was a big performance issue for that package.
addr_of_mut!((*p).need_stat).write(core::cell::Cell::new(found_kind.is_none()));
addr_of_mut!((*p).need_stat).write(AtomicBool::new(found_kind.is_none()));
addr_of_mut!((*p).cache).write(core::cell::Cell::new(EntryCache {
symlink: Interned::EMPTY,
// if found_kind is null, we have set need_stat above, so we
Expand Down
Loading
Loading