Skip to content
Open
Show file tree
Hide file tree
Changes from 6 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
1 change: 1 addition & 0 deletions src/codegen/generate-js2native.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ const rustIdentifierPaths: Record<string, string> = {
"escapeRegExp.rs": "string/escapeRegExp.rs",
"event_loop.rs": "jsc/event_loop.rs",
"ffi.rs": "runtime/ffi/ffi.rs",
"filesystem_router.rs": "runtime/api/filesystem_router.rs",
"h2_frame_parser.rs": "runtime/api/bun/h2_frame_parser.rs",
"hosted_git_info.rs": "install/hosted_git_info.rs",
"http/H2Client.rs": "http/H2Client.rs",
Expand Down
2 changes: 2 additions & 0 deletions src/js/internal-for-testing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ const shellParse = $newRustFunction("shell.rs", "TestingAPIs.shellParse", 2);

export const sslCtxLiveCount = $newRustFunction("SecureContext.rs", "jsLiveCount", 0);

export const dirnameStoreAppendCount = $newRustFunction("filesystem_router.rs", "jsDirnameStoreAppendCount", 0);

export const napiThreadsafeFunctionLiveCount = $newRustFunction("napi_body.rs", "jsThreadsafeFunctionLiveCount", 0);

export const escapeRegExp = $newRustFunction("escapeRegExp.rs", "jsEscapeRegExp", 1);
Expand Down
9 changes: 9 additions & 0 deletions src/resolver/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,15 @@ pub mod fs {
// SAFETY: see `append_slice`.
unsafe { &*$backing() }.exists(value)
}
/// Total number of `append*` calls (inline + overflow).
/// Exposed via `bun:internal-for-testing` so leak tests can
/// assert "N reloads performed zero new appends".
pub fn append_count(&self) -> u32 {
// SAFETY: see `append_slice`.
let b = unsafe { &*$backing() };
let _g = b.mutex.lock();
b.slice_buf_used as u32 + b.overflow_list.count
}
}
};
}
Expand Down
63 changes: 39 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,37 @@ 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] {
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(FileSystem::get().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
12 changes: 12 additions & 0 deletions src/runtime/api/filesystem_router.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,18 @@ pub use crate::bake::framework_router::JSFrameworkRouter as FrameworkFileSystemR

pub(crate) const DEFAULT_EXTENSIONS: &[&[u8]] = &[b"tsx", b"jsx", b"ts", b"mjs", b"cjs", b"js"];

/// Exposed via `bun:internal-for-testing` so leak tests can assert
/// `reload()` performed zero new `DirnameStore` appends.
#[bun_jsc::host_fn]
pub fn js_dirname_store_append_count(
_global: &JSGlobalObject,
_callframe: &CallFrame,
) -> JsResult<JSValue> {
Ok(JSValue::js_number(
Fs::DirnameStore::instance().append_count() as f64,
))
}

// ── local shims ───────────────────────────────────────────────────────────
// `to_js` lives on the `bun_jsc::ZigStringJsc` extension trait; `from_bytes`
// auto-detects UTF-8.
Expand Down
32 changes: 32 additions & 0 deletions test/js/bun/util/filesystem_router.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { FileSystemRouter } from "bun";
import { dirnameStoreAppendCount } from "bun:internal-for-testing";
import { expect, it } from "bun:test";
import fs, { mkdirSync, rmSync } from "fs";
import { bunEnv, bunExe, isASAN, isMacOS, isWindows, normalizeBunSnapshot, tempDir, tmpdirSync } from "harness";
Expand Down Expand Up @@ -402,6 +403,37 @@ it("reload() works", () => {
expect(router.match("/posts")!.name).toBe("/posts");
});

it("reload() does not leak route paths into the process-global intern store", () => {
// Route::parse interns each route's public path, absolute path, basename and
// lowercased match name 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 routes = 40;
const reloads = 50;
const files: Record<string, string> = { "pages/sub/Nested.tsx": "export default 1;\n" };
for (let i = 0; i < routes; i++) files[`pages/Page${i}.tsx`] = "export default 1;\n";
using dir = tempDir("fsr-reload-intern", files);

const router = new Bun.FileSystemRouter({
dir: path.join(String(dir), "pages"),
style: "nextjs",
fileExtensions: [".tsx"],
});
router.reload();
const before = dirnameStoreAppendCount();
for (let i = 0; i < reloads; i++) router.reload();
const delta = dirnameStoreAppendCount() - before;
// Route::parse itself must perform zero new appends after the first reload.
// The resolver's bust-then-reread path still interns a couple of directory
// paths per reload (tracked separately), so allow O(dirs) residual but fail
// on any O(routes) growth. Without the fix delta is routes * 4 * reloads
// (~8000 here); with it, ~6 * reloads.
expect(delta).toBeLessThan(routes * reloads);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
expect(router.match("/Page7")?.filePath).toEndWith("Page7.tsx");
expect(router.match("/sub/Nested")?.filePath).toEndWith("Nested.tsx");
});

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

Expand Down
Loading