diff --git a/src/resolver/dir_info.rs b/src/resolver/dir_info.rs index 07a5f38accf5..b05a752be91f 100644 --- a/src/resolver/dir_info.rs +++ b/src/resolver/dir_info.rs @@ -226,16 +226,10 @@ impl DirInfo { pub fn get_file_descriptor(&self) -> Fd { if FeatureFlags::STORE_FILE_DESCRIPTORS { - // Route through `entries_at` directly (returns `Option<&mut EntriesOption>`) - // instead of round-tripping the safe `&mut DirEntry` through `get_entries`'s - // `*mut` return just to deref it back here. With `generation = 0` the - // generation-check re-read in `entries_at` is a no-op, so this is the - // same lookup `get_entries(0)` performs. - if let Some(fs::EntriesOption::Entries(entries)) = - fs::FileSystem::instance().fs.entries_at(self.entries, 0) - { - return entries.fd; - } + // `entries_at(_, 0)` never re-reads (`u16 < 0` is always false), so the + // lock it would take covers no mutation; go through the same plain + // `at_index` lookup `get_entries_const` uses. + return self.get_entries_const().map_or(Fd::INVALID, |e| e.fd); } Fd::INVALID } @@ -270,6 +264,19 @@ impl DirInfo { } } + /// [`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 fn get_entries_ref_locked(&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), + fs::EntriesOption::Err(_) => None, + } + } + pub fn get_entries_const(&self) -> Option<&fs::DirEntry> { let entries_ptr = fs::FileSystem::instance() .fs diff --git a/src/resolver/lib.rs b/src/resolver/lib.rs index 7c33992c09f0..7658a68ba4e8 100644 --- a/src/resolver/lib.rs +++ b/src/resolver/lib.rs @@ -1597,14 +1597,37 @@ pub mod fs { /// Index lookup with generation-check /// re-read (open + readdir + cache replace) when the cached listing is stale. + /// + /// Takes `entries_mutex` for the whole lookup: the generation-stale branch + /// drops the existing `DirEntry` (and the bucket allocation behind its + /// `data` map) in place, and the route loaders iterate that map under the + /// same lock. Call [`entries_at_locked`](Self::entries_at_locked) instead + /// from inside a critical section that already holds `entries_mutex`. pub fn entries_at( &mut self, index: bun_alloc::IndexType, generation: Generation, ) -> Option<&mut EntriesOption> { + // `MutexGuard` stores the mutex by raw pointer (see `EntriesGuard`), + // so holding it does not keep `&mut self` borrowed. + let _g = self.entries_mutex.lock_guard(); + self.entries_at_locked(index, generation) + } + + /// [`entries_at`](Self::entries_at) for call sites that already hold + /// `entries_mutex` (the mutex is non-recursive). + pub fn entries_at_locked( + &mut self, + index: bun_alloc::IndexType, + generation: Generation, + ) -> Option<&mut EntriesOption> { + debug_assert!( + self.entries_mutex.is_held_by_current_thread(), + "entries_at_locked: caller must hold entries_mutex", + ); // erase to raw immediately so re-borrowing `&mut self` for // `open_dir`/`readdir`/`read_directory_error` doesn't conflict. - // `entries_mutex` held by caller; sole `&mut` to this slot. + // `entries_mutex` held (by `entries_at` or the caller); sole `&mut` to this slot. let result_ptr = std::ptr::from_mut::(self.entries.at_index(index)?); // SAFETY: BSSMap-owned slot; uniquely held under `entries_mutex`. if let EntriesOption::Entries(existing) = unsafe { &mut *result_ptr } { diff --git a/src/resolver/resolver.rs b/src/resolver/resolver.rs index b0ef596c5453..24b6988048f0 100644 --- a/src/resolver/resolver.rs +++ b/src/resolver/resolver.rs @@ -3466,7 +3466,10 @@ impl<'a> Resolver<'a> { unsafe { &mut *rfs } }; } - // resolver mutex held; `EntriesMap` methods are safe wrappers over the singleton. + // Hold `entries_mutex` across the in-place `DirEntry` rewrite below and + // the `dir_info_uncached` call, mirroring `dir_info_cached_miss`: the + // route loaders iterate the `DirEntry.data` map under this lock. + let _entries_unlock = rfs!().entries_mutex.lock_guard(); let mut cached_dir_entry_result = rfs!().entries.get_or_put(dir_path)?; // NOTE: always assigned by either the cached-hit arm or the @@ -6201,7 +6204,10 @@ impl<'a> Resolver<'a> { // Make sure "absRealPath" is the real path of the directory (resolving any symlinks) if !self.opts.preserve_symlinks { - if let Some(parent_entries) = parent_.get_entries_ref(self.generation) { + // The only caller that reaches this with `parent` set + // (`dir_info_cached_miss`) already holds `entries_mutex`, and that + // mutex is non-recursive, so go through the `_locked` accessor. + if let Some(parent_entries) = parent_.get_entries_ref_locked(self.generation) { if let Some(lookup) = parent_entries.get(base) { let entries_fd = entries!().fd; if entries_fd.is_valid() diff --git a/test/js/bun/util/filesystem_router.test.ts b/test/js/bun/util/filesystem_router.test.ts index 65d3390e3293..a815be1d9c7d 100644 --- a/test/js/bun/util/filesystem_router.test.ts +++ b/test/js/bun/util/filesystem_router.test.ts @@ -836,17 +836,26 @@ it("reload() while Bun.build() resolves the same directory", async () => { style: "nextjs", fileExtensions: [".tsx"], }); - const builds = Array.from({ length: 4 }, () => - Bun.build({ entrypoints, target: "bun", throw: false }), - ); + // The first build completes with generation 0 and the bundle thread then + // bumps its generation, so every later build's resolver re-reads the + // directory listing in place. reload() iterates the same listing on the + // main thread, and that in-place re-read is what the reload loop races. + await Bun.build({ entrypoints, target: "bun", throw: false }); let matches = 0; - for (let i = 0; i < 50; i++) { - router.reload(); - const m = router.match("/p7"); - if (m && m.filePath.endsWith("p7.tsx")) matches++; + let buildsOk = true; + for (let round = 0; round < 40; round++) { + const builds = Array.from({ length: 4 }, () => + Bun.build({ entrypoints, target: "bun", throw: false }), + ); + for (let i = 0; i < 50; i++) { + router.reload(); + const m = router.match("/p7"); + if (m && m.filePath.endsWith("p7.tsx")) matches++; + } + const results = await Promise.all(builds); + buildsOk &&= results.every(r => r.success); } - const results = await Promise.all(builds); - console.log("matches", matches, "builds-ok", results.every(r => r.success)); + console.log("matches", matches, "builds-ok", buildsOk); `, }; for (let i = 1; i <= 40; i++) { @@ -863,8 +872,12 @@ it("reload() while Bun.build() resolves the same directory", async () => { stderr: "pipe", }); const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - expect(normalizeBunSnapshot(stdout, String(dir))).toBe("matches 50 builds-ok true"); - expect({ exitCode, signalCode: proc.signalCode }).toEqual({ exitCode: 0, signalCode: null }); + expect({ + stdout: normalizeBunSnapshot(stdout, String(dir)), + stderr: normalizeBunSnapshot(stderr, String(dir)), + exitCode, + signalCode: proc.signalCode, + }).toEqual({ stdout: "matches 2000 builds-ok true", stderr: "", exitCode: 0, signalCode: null }); }, 60_000); it("loads routes from a directory already cached by Bun.build()", async () => { @@ -897,6 +910,10 @@ it("loads routes from a directory already cached by Bun.build()", async () => { stderr: "pipe", }); const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - expect(normalizeBunSnapshot(stdout, String(dir))).toBe("/a /b /sub/c /b"); - expect({ exitCode, signalCode: proc.signalCode }).toEqual({ exitCode: 0, signalCode: null }); + expect({ + stdout: normalizeBunSnapshot(stdout, String(dir)), + stderr: normalizeBunSnapshot(stderr, String(dir)), + exitCode, + signalCode: proc.signalCode, + }).toEqual({ stdout: "/a /b /sub/c /b", stderr: "", exitCode: 0, signalCode: null }); });