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
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