Skip to content
Open
Show file tree
Hide file tree
Changes from 2 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
68 changes: 44 additions & 24 deletions src/router/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1123,18 +1123,14 @@ impl Route {
let name_offset = name.as_ptr() as usize - public_path.as_ptr() as usize;
let name_len = name.len();

// NOTE: DirnameStore::append returns `&'static [u8]` (process-
// NOTE: `intern_route_path` returns `&'static [u8]` (process-
// lifetime arena), so rebinding here drops the borrow on
// `route_file_buf` and avoids needing lifetime transmutes
// below.
let dirname_store = FileSystem::instance().dirname_store();
let public_path: &'static [u8] =
dirname_store.append(public_path).expect("unreachable");
let public_path: &'static [u8] = intern_route_path(public_path);
let name: &'static [u8] = &public_path[name_offset..][0..name_len];
let match_name: &'static [u8] = if has_uppercase {
dirname_store
.append_lower_case(&name[1..])
.expect("unreachable")
intern_route_path_lower_case(&name[1..])
} else {
&name[1..]
};
Expand All @@ -1143,9 +1139,7 @@ impl Route {
debug_assert!(name[0] == b'/');
(public_path, name, match_name)
} else {
let dirname_store = FileSystem::instance().dirname_store();
let public_path: &'static [u8] =
dirname_store.append(public_path).expect("unreachable");
let public_path: &'static [u8] = intern_route_path(public_path);
(
public_path,
Route::INDEX_ROUTE_NAME,
Expand Down Expand Up @@ -1231,10 +1225,7 @@ impl Route {
}
};

abs_path_str = FileSystem::instance()
.dirname_store()
.append(_abs)
.expect("unreachable");
abs_path_str = intern_route_path(_abs);

// SAFETY: sole mutation; `base_`/`extname` (which may borrow
// `(*entry).base_.remainder_buf`) are not used after this.
Expand All @@ -1249,11 +1240,7 @@ impl Route {
abs_path_str,
&mut bufs.normalized_abs_path_buf,
);
let interned: &'static [u8] = FileSystem::instance()
.dirname_store()
.append(normalized)
.expect("unreachable");
Interned::from_static(interned)
Interned::from_static(intern_route_path(normalized))
};
#[cfg(not(windows))]
let abs_path = Interned::from_static(abs_path_str);
Expand All @@ -1269,14 +1256,11 @@ impl Route {
}

// NOTE: name/match_name/public_path are already `&'static` via
// DirnameStore::append above. `entry.base()` borrows the entry (it
// `intern_route_path` above. `entry.base()` borrows the entry (it
// may be inline-stored for ≤31-byte names); intern it
// explicitly to get `&'static` without a lifetime transmute.
// SAFETY: read-only reborrow; the `&mut` write above is dead.
let basename: &'static [u8] = FileSystem::instance()
.dirname_store()
.append(unsafe { &*entry }.base())
.expect("unreachable");
let basename: &'static [u8] = intern_route_path(unsafe { &*entry }.base());

Some(Route {
name,
Expand Down Expand Up @@ -1381,6 +1365,42 @@ thread_local! {
#[cfg(windows)]
normalized_abs_path_buf: bun_sys::windows::PathBuffer::ZEROED,
}));

static ROUTE_PATH_INTERN: RefCell<bun_collections::HashMap<&'static [u8], ()>> =
RefCell::new(bun_collections::HashMap::new());
}

/// Intern `value` into the process-lifetime `DirnameStore`, deduplicated by
/// content. `BSSStringList::append` does not dedupe, so without this every
/// `FileSystemRouter::reload()` would re-append each route's public path,
/// absolute path and basename, eventually exhausting the store's slot capacity
/// (`AllocError`) and leaking one heap buffer per append. Same pattern as
/// `intern_transpile_path` in `jsc_hooks.rs`.
fn intern_route_path(value: &[u8]) -> &'static [u8] {
let dirname_store = FileSystem::instance().dirname_store();
if dirname_store.exists(value) {
// SAFETY: `exists` is a pointer-range check against the store's
// process-lifetime backing buffer, so `value` is already `'static`.
return unsafe { core::slice::from_raw_parts(value.as_ptr(), value.len()) };
}
ROUTE_PATH_INTERN.with_borrow_mut(|set| {
if let Some((interned, ())) = set.get_key_value(value) {
return *interned;
}
let interned: &'static [u8] = bun_core::handle_oom(dirname_store.append(value));
set.insert(interned, ());
interned
})
}

fn intern_route_path_lower_case(value: &[u8]) -> &'static [u8] {
if value.len() <= 256 {
let mut scratch = [0u8; 256];
intern_route_path(strings::copy_lowercase(value, &mut scratch[..value.len()]))
} else {
let mut scratch = vec![0u8; value.len()];
intern_route_path(strings::copy_lowercase(value, &mut scratch))
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

pub struct Match<'a> {
Expand Down
53 changes: 52 additions & 1 deletion test/js/bun/util/filesystem_router.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { FileSystemRouter } from "bun";
import { expect, it } from "bun:test";
import fs, { mkdirSync, rmSync } from "fs";
import { bunEnv, bunExe, isASAN, isMacOS, isWindows, normalizeBunSnapshot, tempDir, tmpdirSync } from "harness";
import { bunEnv, bunExe, isASAN, isLinux, isMacOS, isWindows, normalizeBunSnapshot, tempDir, tmpdirSync } from "harness";
import path, { dirname } from "path";

function createTree(basedir: string, paths: string[]) {
Expand Down Expand Up @@ -402,6 +402,57 @@ it("reload() works", () => {
expect(router.match("/posts")!.name).toBe("/posts");
});

// The deep-path fixture needs ~1500-byte absolute paths so the leaked abs_path
// interns dominate RSS over the unrelated per-reload DirEntry churn; Linux's
// PATH_MAX=4096 fits that, macOS's (1024) does not, and running 200*400 file
// opens through Windows is too slow for CI. The leak and fix are
// platform-independent (Route::parse).
it.skipIf(!isLinux)("reload() does not leak route paths into the process-global intern store", async () => {
// Route::parse interns each route's public path, absolute path, and basename
// into the never-freed DirnameStore. Without content dedup, every reload()
// re-appended identical strings, leaking one heap buffer per append and
// eventually panicking with `unreachable: AllocError` when the store's slot
// capacity was exhausted.
const seg = Buffer.alloc(100, "d").toString();
const parts = Array.from({ length: 14 }, () => seg);
const files: Record<string, string> = {};
for (let i = 0; i < 200; i++) files[[...parts, "pages", `Page${i}.tsx`].join("/")] = "export default 1;\n";
using dir = tempDir("fsr-reload-intern", files);
const pagesDir = path.join(String(dir), ...parts, "pages");

const code = /* ts */ `
const router = new Bun.FileSystemRouter({
dir: ${JSON.stringify(pagesDir)},
style: "nextjs",
fileExtensions: [".tsx"],
});
const m = router.match("/Page7");
if (!m || !m.filePath.endsWith("Page7.tsx")) throw new Error("match() broken: " + m?.filePath);
for (let i = 0; i < 5; i++) router.reload();
Bun.gc(true);
const before = process.memoryUsage.rss();
for (let i = 0; i < 400; i++) router.reload();
Bun.gc(true);
const m2 = router.match("/Page7");
if (!m2 || !m2.filePath.endsWith("Page7.tsx")) throw new Error("match() broken after reload: " + m2?.filePath);
const growthMB = (process.memoryUsage.rss() - before) / 1024 / 1024;
console.error("RSS growth: " + growthMB.toFixed(1) + "MB");
if (growthMB > 110) throw new Error("leaked " + growthMB.toFixed(1) + "MB");
`;

await using proc = Bun.spawn({
cmd: [bunExe(), "--smol", "-e", code],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).not.toContain("leaked");
expect(stderr).not.toContain("AllocError");
expect(stdout).toBe("");
expect(exitCode).toBe(0);
}, 60_000);

it("reload() works with new dirs/files", () => {
const { dir } = make(["posts.tsx"]);

Expand Down
Loading