diff --git a/src/jsc/hot_reloader.rs b/src/jsc/hot_reloader.rs index 6e66a67d0395..6c4c7c14dc7c 100644 --- a/src/jsc/hot_reloader.rs +++ b/src/jsc/hot_reloader.rs @@ -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 { diff --git a/src/resolver/fs.rs b/src/resolver/fs.rs index 5bce1207b0ca..07e5ed0a973c 100644 --- a/src/resolver/fs.rs +++ b/src/resolver/fs.rs @@ -279,7 +279,7 @@ pub struct Entry { pub mutex: Mutex, pub need_stat: core::cell::Cell, - pub abs_path: Interned, + pub abs_path: core::cell::Cell, } impl Entry { @@ -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. @@ -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()), } } } @@ -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), } } } @@ -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 } }; diff --git a/src/resolver/resolver.rs b/src/resolver/resolver.rs index 24b6988048f0..d28d7f7940da 100644 --- a/src/resolver/resolver.rs +++ b/src/resolver/resolver.rs @@ -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 { @@ -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!( @@ -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, @@ -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, @@ -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, diff --git a/src/router/lib.rs b/src/router/lib.rs index 797365348028..b5c53b79e9bf 100644 --- a/src/router/lib.rs +++ b/src/router/lib.rs @@ -1026,18 +1026,14 @@ impl Route { ) -> Option { // 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()]; @@ -1153,12 +1149,17 @@ impl Route { ) }; - 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(); + 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; `needs_close` is a // Cell so the drop closure can read it while the body still mutates. @@ -1236,9 +1237,8 @@ impl Route { .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)] diff --git a/src/runtime/cli/test/Scanner.rs b/src/runtime/cli/test/Scanner.rs index dd616fc021e1..babb875b4662 100644 --- a/src/runtime/cli/test/Scanner.rs +++ b/src/runtime/cli/test/Scanner.rs @@ -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; } @@ -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()); } } } diff --git a/test/js/bun/util/filesystem_router.test.ts b/test/js/bun/util/filesystem_router.test.ts index a815be1d9c7d..a91f10586ab0 100644 --- a/test/js/bun/util/filesystem_router.test.ts +++ b/test/js/bun/util/filesystem_router.test.ts @@ -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")); @@ -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 }), @@ -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"); + } 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++) { @@ -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 () => {