Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 1 addition & 1 deletion src/jsc/hot_reloader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1201,8 +1201,8 @@ where
let _entry_guard = ent.mutex.lock_guard();
ent.set_cache_fd(Fd::INVALID);
ent.need_stat.set(true);
path_string = ent.abs_path();
}
path_string = ent.abs_path;
file_hash = Watcher::get_hash(path_string.as_bytes());
for (entry_id, hash) in hashes.iter().enumerate() {
if *hash == file_hash {
Expand Down
33 changes: 25 additions & 8 deletions src/resolver/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ pub struct Entry {
pub mutex: Mutex,
pub need_stat: core::cell::Cell<bool>,

pub abs_path: Interned,
pub abs_path: core::cell::Cell<Interned>,
}

impl Entry {
Expand Down Expand Up @@ -336,15 +336,32 @@ impl Entry {
self.dir
}

/// `Interned` is `Copy`.
/// `Interned` is `Copy`. Caller must hold `self.mutex` when this entry is
/// reachable from another thread; see [`abs_path_or_fill`].
#[inline]
pub fn abs_path(&self) -> Interned {
self.abs_path
self.abs_path.get()
}

#[inline]
pub fn set_abs_path(&mut self, p: Interned) {
self.abs_path = p;
pub fn set_abs_path(&self, p: Interned) {
self.abs_path.set(p);
}

/// Double-checked lazy fill of `abs_path` under `self.mutex`, matching
/// [`kind`](Self::kind) / [`symlink`](Self::symlink). Returns the cached
/// value if already set, otherwise calls `fill`, stores the result, and
/// returns it. `abs_path` is a two-word slice, so unlocked access can
/// observe a torn `(ptr, len)` on weakly-ordered CPUs.
pub fn abs_path_or_fill(&self, fill: impl FnOnce() -> Interned) -> &'static [u8] {
let _g = self.mutex.lock_guard();
let cached = self.abs_path.get();
if !cached.is_empty() {
return cached.as_bytes();
}
let v = fill();
self.abs_path.set(v);
v.as_bytes()
}

/// Stat-on-first-use.
Expand Down Expand Up @@ -426,7 +443,7 @@ impl Clone for Entry {
base_lowercase_: strings::StringOrTinyString::init(self.base_lowercase_.slice()),
mutex: Mutex::default(),
need_stat: core::cell::Cell::new(self.need_stat.get()),
abs_path: self.abs_path,
abs_path: core::cell::Cell::new(self.abs_path.get()),
}
}
}
Expand All @@ -440,7 +457,7 @@ impl Default for Entry {
base_lowercase_: strings::StringOrTinyString::init(b""),
mutex: Mutex::default(),
need_stat: core::cell::Cell::new(true),
abs_path: Interned::EMPTY,
abs_path: core::cell::Cell::new(Interned::EMPTY),
}
}
}
Expand Down Expand Up @@ -731,7 +748,7 @@ impl DirEntry {
kind: found_kind.unwrap_or(EntryKind::File),
fd: Fd::INVALID,
}));
addr_of_mut!((*p).abs_path).write(Interned::EMPTY);
addr_of_mut!((*p).abs_path).write(core::cell::Cell::new(Interned::EMPTY));
p
}
};
Expand Down
151 changes: 59 additions & 92 deletions src/resolver/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3895,19 +3895,14 @@ impl<'a> Resolver<'a> {
return MatchStatus::NotFound;
}

let absolute_out_path: &[u8] = {
if entry_query.entry().abs_path.is_empty() {
// SAFETY: EntryStore-owned slot; resolver mutex held. RHS fully
// evaluated before LHS `&mut Entry` is materialized.
unsafe { &mut *entry_query.entry }.abs_path = Interned::from_static(
self.fs_ref()
.dirname_store
.append_slice(abs_esm_path)
.expect("unreachable"),
);
}
entry_query.entry().abs_path.as_bytes()
};
let absolute_out_path: &[u8] = entry_query.entry().abs_path_or_fill(|| {
Interned::from_static(
self.fs_ref()
.dirname_store
.append_slice(abs_esm_path)
.expect("unreachable"),
)
});
let module_type = if let Some(pkg) = resolved_dir_info.package_json() {
pkg.module_type
} else {
Expand Down Expand Up @@ -5270,21 +5265,16 @@ impl<'a> Resolver<'a> {
if unsafe { lookup.entry().kind(rfs, self.store_fd) }
== Fs::file_system::EntryKind::File
{
let out_buf: &[u8] = {
if lookup.entry().abs_path.is_empty() {
let parts = [dir_info.abs_path, &base[..]];
let out_buf_ = self.fs_ref().abs_buf(&parts, bufs!(index));
// SAFETY: EntryStore-owned slot; resolver mutex held. RHS fully
// evaluated before LHS `&mut Entry` is materialized.
unsafe { &mut *lookup.entry }.abs_path = Interned::from_static(
self.fs_ref()
.dirname_store
.append_slice(out_buf_)
.expect("unreachable"),
);
}
lookup.entry().abs_path.as_bytes()
};
let out_buf: &[u8] = lookup.entry().abs_path_or_fill(|| {
let parts = [dir_info.abs_path, &base[..]];
let out_buf_ = self.fs_ref().abs_buf(&parts, bufs!(index));
Interned::from_static(
self.fs_ref()
.dirname_store
.append_slice(out_buf_)
.expect("unreachable"),
)
});

if let Some(debug) = self.debug_logs.as_mut() {
debug.add_note_fmt(format_args!(
Expand Down Expand Up @@ -5767,21 +5757,16 @@ impl<'a> Resolver<'a> {
debug.add_note_fmt(format_args!("Found file \"{}\" ", bstr::BStr::new(base)));
}

let abs_path: &'static [u8] = {
if query.entry().abs_path.is_empty() {
let abs_path_parts = [query.entry().dir, query.entry().base()];
let joined = self.fs_ref().abs_buf(&abs_path_parts, bufs!(load_as_file));
// SAFETY: EntryStore-owned slot; resolver mutex held. RHS fully
// evaluated before LHS `&mut Entry` is materialized.
unsafe { &mut *query.entry }.abs_path = Interned::from_static(
self.fs_ref()
.dirname_store
.append_slice(joined)
.expect("unreachable"),
);
}
query.entry().abs_path.as_bytes()
};
let abs_path: &'static [u8] = query.entry().abs_path_or_fill(|| {
let abs_path_parts = [query.entry().dir, query.entry().base()];
let joined = self.fs_ref().abs_buf(&abs_path_parts, bufs!(load_as_file));
Interned::from_static(
self.fs_ref()
.dirname_store
.append_slice(joined)
.expect("unreachable"),
)
});

dec_ret!(Some(LoadResult {
path: abs_path,
Expand Down Expand Up @@ -5873,39 +5858,30 @@ impl<'a> Resolver<'a> {
}

dec_ret!(Some(LoadResult {
path: {
if query.entry().abs_path.is_empty() {
// SAFETY: `dir` is `&'static [u8]` (DirnameStore-interned),
// copied out so no `&Entry` borrow survives into the
// `&mut Entry` write below.
let entry_dir = query.entry().dir;
let new_abs = if !entry_dir.is_empty()
&& entry_dir[entry_dir.len() - 1] == SEP
{
let parts: [&[u8]; 2] = [entry_dir, &buffer[..]];
Interned::from_static(
self.fs_ref()
.filename_store
.append_parts(&parts)
.expect("unreachable"),
)
// the trailing path CAN be missing here
} else {
let parts: [&[u8]; 3] =
[entry_dir, SEP_STR.as_bytes(), &buffer[..]];
Interned::from_static(
self.fs_ref()
.filename_store
.append_parts(&parts)
.expect("unreachable"),
)
};
// SAFETY: EntryStore-owned slot; resolver mutex held. RHS
// fully evaluated above — sole `&mut Entry` for this write.
unsafe { &mut *query.entry }.abs_path = new_abs;
path: query.entry().abs_path_or_fill(|| {
let entry_dir = query.entry().dir;
if !entry_dir.is_empty()
&& entry_dir[entry_dir.len() - 1] == SEP
{
let parts: [&[u8]; 2] = [entry_dir, &buffer[..]];
Interned::from_static(
self.fs_ref()
.filename_store
.append_parts(&parts)
.expect("unreachable"),
)
// the trailing path CAN be missing here
} else {
let parts: [&[u8]; 3] =
[entry_dir, SEP_STR.as_bytes(), &buffer[..]];
Interned::from_static(
self.fs_ref()
.filename_store
.append_parts(&parts)
.expect("unreachable"),
)
}
query.entry().abs_path.as_bytes()
},
}),
diff_case: query.diff_case,
dirname_fd: entries!().fd,
file_fd: query.entry().cache().fd,
Expand Down Expand Up @@ -5980,23 +5956,14 @@ impl<'a> Resolver<'a> {

// now that we've found it, we allocate it.
return Some(LoadResult {
path: {
// SAFETY: EntryStore-owned slot; resolver mutex held. RHS is fully
// evaluated (shared reads) before the LHS `&mut Entry` is
// materialized for the write — no overlapping unique borrow.
unsafe { &mut *query.entry }.abs_path = if query.entry().abs_path.is_empty()
{
Interned::from_static(
self.fs_ref()
.dirname_store
.append_slice(&buffer[..])
.expect("unreachable"),
)
} else {
query.entry().abs_path
};
query.entry().abs_path.as_bytes()
},
path: query.entry().abs_path_or_fill(|| {
Interned::from_static(
self.fs_ref()
.dirname_store
.append_slice(&buffer[..])
.expect("unreachable"),
)
}),
diff_case: query.diff_case,
dirname_fd: entries.fd,
file_fd: query.entry().cache().fd,
Expand Down
28 changes: 14 additions & 14 deletions src/router/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1026,18 +1026,14 @@
) -> Option<Route> {
// NOTE: `entry` is a raw `*mut Entry`
// because `base_`/`extname` may borrow `(*entry).base_` (tiny inline
// string) and a `&mut Entry` parameter would alias them.
// Reads go through `unsafe { &*entry }`; the single mutation
// (`set_abs_path`) goes through `unsafe { &mut *entry }` after
// `base_`/`extname` are no longer used.
// string) and a `&mut Entry` parameter would alias them. Reads and the
// `abs_path` fill go through `unsafe { &*entry }`.
// SAFETY: caller passes an EntryStore-owned pointer valid for the
// process lifetime; no other live `&mut` to it during this call.
let entry_abs_path = unsafe { &*entry }.abs_path().as_bytes();
let mut abs_path_str: &[u8] = if entry_abs_path.is_empty() {
b""
} else {
entry_abs_path
};
// `abs_path` is lazily filled under `Entry.mutex` in the `'fill` block
// below (see `Entry::abs_path_or_fill`); start empty and let that
// locked check populate it so the hot path takes the lock once.
let mut abs_path_str: &[u8] = b"";

let base = &base_[0..base_.len() - extname.len()];

Expand Down Expand Up @@ -1153,12 +1149,17 @@
)
};

if abs_path_str.is_empty() {
'fill: {
// The reads of `cache().fd` and the `set_abs_path` write below
// rewrite the cached `Entry`; serialize them on the per-entry
// mutex (the same lock every other `Entry` rewrite path takes).
// SAFETY: see fn-level NOTE — read-only reborrow.
let _entry_guard = unsafe { &*entry }.mutex.lock_guard();
let cached = unsafe { &*entry }.abs_path();

Check failure on line 1158 in src/router/lib.rs

View workflow job for this annotation

GitHub Actions / cargo clippy

unsafe block missing a safety comment
if !cached.is_empty() {
abs_path_str = cached.as_bytes();
break 'fill;
}
// NOTE: reshaped for borrowck — `defer if (needs_close) file.close()`
// becomes a scopeguard owning the Option<File>; `needs_close` is a
// Cell so the drop closure can read it while the body still mutates.
Expand Down Expand Up @@ -1236,9 +1237,8 @@
.append(_abs)
.expect("unreachable");

// SAFETY: sole mutation; `base_`/`extname` (which may borrow
// `(*entry).base_.remainder_buf`) are not used after this.
unsafe { &mut *entry }.set_abs_path(Interned::from_static(abs_path_str));
// SAFETY: see fn-level NOTE — read-only reborrow; `Entry.mutex` held.
unsafe { &*entry }.set_abs_path(Interned::from_static(abs_path_str));
}

#[cfg(windows)]
Expand Down
6 changes: 3 additions & 3 deletions src/runtime/cli/test/Scanner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ impl<'a> Scanner<'a> {
}
fs::EntryKind::File => {
// already seen it!
if !entry.abs_path.is_empty() {
if !entry.abs_path().is_empty() {
return;
}

Expand Down Expand Up @@ -450,8 +450,8 @@ impl<'a> Scanner<'a> {
Ok(s) => s,
Err(_) => bun_core::out_of_memory(),
};
entry.abs_path = Interned::from_static(stored);
self.test_files.push(entry.abs_path);
entry.set_abs_path(Interned::from_static(stored));
self.test_files.push(entry.abs_path());
}
}
}
Expand Down
13 changes: 11 additions & 2 deletions test/js/bun/util/filesystem_router.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -826,6 +826,7 @@ it("reload() while Bun.build() resolves the same directory", async () => {
"fixture.ts": /* ts */ `
import path from "path";
const pagesDir = path.join(import.meta.dir, "pages");
const pagesDirPosix = pagesDir.replaceAll(path.sep, "/");
const entrypoints: string[] = [];
for (let i = 1; i <= 40; i++) {
entrypoints.push(path.join(pagesDir, "p" + i + ".tsx"));
Expand All @@ -843,6 +844,7 @@ it("reload() while Bun.build() resolves the same directory", async () => {
await Bun.build({ entrypoints, target: "bun", throw: false });
let matches = 0;
let buildsOk = true;
let pathsOk = true;
for (let round = 0; round < 40; round++) {
const builds = Array.from({ length: 4 }, () =>
Bun.build({ entrypoints, target: "bun", throw: false }),
Expand All @@ -852,10 +854,17 @@ it("reload() while Bun.build() resolves the same directory", async () => {
const m = router.match("/p7");
if (m && m.filePath.endsWith("p7.tsx")) matches++;
}
// Each route's abs-path is filled by both the router (under the
// per-entry mutex) and the bundler's resolver for the same fresh
// Entry after every bust+reread; a torn value surfaces as a
// filePath that isn't the absolute .tsx path.
for (const fp of Object.values(router.routes)) {
pathsOk &&= typeof fp === "string" && fp.startsWith(pagesDirPosix) && fp.endsWith(".tsx");
}
Comment thread
robobun marked this conversation as resolved.
const results = await Promise.all(builds);
buildsOk &&= results.every(r => r.success);
}
console.log("matches", matches, "builds-ok", buildsOk);
console.log("matches", matches, "builds-ok", buildsOk, "paths-ok", pathsOk);
`,
};
for (let i = 1; i <= 40; i++) {
Expand All @@ -877,7 +886,7 @@ it("reload() while Bun.build() resolves the same directory", async () => {
stderr: normalizeBunSnapshot(stderr, String(dir)),
exitCode,
signalCode: proc.signalCode,
}).toEqual({ stdout: "matches 2000 builds-ok true", stderr: "", exitCode: 0, signalCode: null });
}).toEqual({ stdout: "matches 2000 builds-ok true paths-ok true", stderr: "", exitCode: 0, signalCode: null });
}, 60_000);

it("loads routes from a directory already cached by Bun.build()", async () => {
Expand Down
Loading