Skip to content
Merged
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
60 changes: 56 additions & 4 deletions src/collections/multi_array_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -991,28 +991,40 @@ impl<T, A: Allocator> MultiArrayList<T, A> {
/// Remove the specified item from the list, swapping the last
/// item in the list into its position. Fast, but does not
/// retain list ordering.
pub fn swap_remove(&mut self, index: usize) {
///
/// Returns the removed element, whose fields the caller now owns (as with
/// [`pop`](Self::pop)). Nothing else ever runs a removed row's destructor:
/// `Drop` for this list is slab-only and
/// [`drop_elements`](Self::drop_elements) only sees rows still in the list.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub fn swap_remove(&mut self, index: usize) -> T {
assert!(
index < self.len,
"MultiArrayList::swap_remove: index out of bounds"
);
let last = self.len - 1;
let mut s = self.slice();
let removed = s.gather(index);
s.copy_rows_within(last, index, 1);
self.len -= 1;
removed
}

/// Remove the specified item from the list, shifting items
/// after it to preserve order.
pub fn ordered_remove(&mut self, index: usize) {
///
/// Returns the removed element; ownership semantics as in
/// [`swap_remove`](Self::swap_remove).
Comment thread
robobun marked this conversation as resolved.
Outdated
pub fn ordered_remove(&mut self, index: usize) -> T {
assert!(
index < self.len,
"MultiArrayList::ordered_remove: index out of bounds"
);
let tail = self.len - 1 - index;
let mut s = self.slice();
let removed = s.gather(index);
s.copy_rows_within(index + 1, index, tail);
self.len -= 1;
removed
}

/// Attempt to reduce allocated capacity to `new_len`.
Expand Down Expand Up @@ -1422,10 +1434,50 @@ mod tests {
.unwrap();
}
assert_eq!(list.items::<"a", u32>(), &[0, 1, 2, 3, 4, 5]);
list.ordered_remove(2);
assert_eq!(list.ordered_remove(2), Foo { a: 2, b: 2, c: 2 });
assert_eq!(list.items::<"a", u32>(), &[0, 1, 3, 4, 5]);
list.swap_remove(1);
assert_eq!(list.swap_remove(1), Foo { a: 1, b: 1, c: 1 });
assert_eq!(list.items::<"a", u32>(), &[0, 5, 3, 4]);
assert_eq!(list.items::<"c", u64>(), &[0, 5, 3, 4]);
// Removing the last row swaps it with itself.
assert_eq!(list.swap_remove(3), Foo { a: 4, b: 4, c: 4 });
assert_eq!(list.items::<"a", u32>(), &[0, 5, 3]);
Comment thread
robobun marked this conversation as resolved.
}

struct Owning {
name: Box<[u8]>,
n: u32,
}

// Under Miri (`bun run rust:miri`) this also checks that a removed row's
// heap payload is freed exactly once: by the caller dropping the returned
// element, not again by `drop_elements`.
Comment thread
robobun marked this conversation as resolved.
Outdated
#[test]
fn remove_returns_owned_element() {
let mut list = MultiArrayList::<Owning>::default();
for i in 0..4u32 {
list.push(Owning {
name: vec![b'a' + i as u8; 3].into_boxed_slice(),
n: i,
})
.unwrap();
}

let removed = list.swap_remove(1);
assert_eq!(&*removed.name, b"bbb");
assert_eq!(removed.n, 1);
assert_eq!(list.items::<"n", u32>(), &[0, 3, 2]);
drop(removed);

let removed = list.ordered_remove(0);
assert_eq!(&*removed.name, b"aaa");
assert_eq!(list.items::<"n", u32>(), &[3, 2]);
assert_eq!(&*list.items::<"name", Box<[u8]>>()[0], b"ddd");
assert_eq!(&*list.items::<"name", Box<[u8]>>()[1], b"ccc");
drop(removed);

list.drop_elements();
assert_eq!(list.len(), 0);
}

#[test]
Expand Down
7 changes: 6 additions & 1 deletion src/watcher/Watcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,9 @@ impl Watcher {
if item == last_item || self.watchlist.len() <= item as usize {
continue;
}
self.watchlist.swap_remove(item as usize);
// Dropping the removed row frees the `file_path` that
// `CLONE_FILE_PATH` entries own; the fd was already closed above.
Comment thread
robobun marked this conversation as resolved.
Outdated
drop(self.watchlist.swap_remove(item as usize));

// swapRemove put a different entry at `item`, but its kqueue registration still
// carries its old `udata` (= pre-swap index). Rewrite it so subsequent kevents
Expand Down Expand Up @@ -1051,6 +1053,9 @@ impl fmt::Display for Op {
// ─── WatchItem ────────────────────────────────────────────────────────────

pub struct WatchItem {
/// `Owned` when the entry was added with `CLONE_FILE_PATH`, and freed when
/// `flush_evictions` removes the entry. Otherwise `Borrowed`, and the caller
/// must keep the bytes alive for as long as the entry exists.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub file_path: Cow<'static, [u8]>,
// filepath hash for quick comparison
pub hash: u32,
Expand Down
63 changes: 63 additions & 0 deletions test/cli/hot/watch-many-dirs.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -182,4 +182,67 @@
},
60000,
);

// Editing dep.js raises an inotify event on lib/, which makes the reloader
// evict dep's watchlist entry; the reload then re-adds it with a fresh heap
// copy of the path. Eviction used to discard the evicted entry's copy
// without freeing it, so every edit leaked one path, which LSan reports when
// the process exits (the same check CI's ASAN lane applies to every test
// process). logLevel=debug makes the reloader log each eviction, so a reload
// cycle that stopped evicting cannot pass this vacuously.
test.skipIf(!isLinux || !isASAN)("evicting watchlist entries does not leak their paths", async () => {

Check warning on line 193 in test/cli/hot/watch-many-dirs.test.ts

View check run for this annotation

Claude / Claude Code Review

New LSan leak test lacks explicit timeout

nit: this test has no per-test timeout, so it inherits the 5s default — but it spawns a debug-ASAN `bun --hot` with `BUN_DESTRUCT_VM_ON_EXIT=1` + `detect_leaks=1`, which `test/no-validate-leaksan.txt` already documents as pushing watch tests past 5s on its own. Both sibling `--hot` subprocess tests in this file set explicit 30000/60000 timeouts; consider adding a third argument (e.g. `}, 30000);`) to match. CI overrides the default via the runner's `--timeout` (with the ASAN multiplier), so this
Comment thread
robobun marked this conversation as resolved.
const edits = 3;
await using dir = tempDir("hot-evict-leak", {
"bunfig.toml": `logLevel = "debug"\n`,
"lib/dep.js": `export const value = 0;`,
"entry.js": `
import { value } from "./lib/dep.js";
console.log("RELOAD", value);
if (value === ${edits}) process.exit(0);
`,
});

await using proc = spawn({
cmd: [bunExe(), "--hot", "entry.js"],
cwd: String(dir),
env: {
...bunEnv,
// Bun's built-in ASAN defaults turn LSan off. Destructing the VM on exit
// frees what JS still referenced, so only lost allocations get reported.
ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "detect_leaks=1"].filter(Boolean).join(":"),
// verbosity=1 makes the exit-time check announce itself on stderr.
LSAN_OPTIONS: [bunEnv.LSAN_OPTIONS, "verbosity=1"].filter(Boolean).join(":"),
BUN_DESTRUCT_VM_ON_EXIT: "1",
},
stdout: "pipe",
stderr: "pipe",
});
const stderrText = proc.stderr.text();

const iter = forEachLine(proc.stdout);
const waitForLine = async (expected: string) => {
while (true) {
const { value: line, done } = await iter.next();
if (done) throw new Error(`--hot exited before printing "${expected}" (exit ${proc.exitCode})`);
if (line === expected) return;
}
};

await waitForLine("RELOAD 0");
for (let i = 1; i <= edits; i++) {
writeFileSync(join(dir, "lib", "dep.js"), `export const value = ${i};`);
await waitForLine(`RELOAD ${i}`);
}
const [stderr, exitCode] = await Promise.all([stderrText, proc.exited]);

// One edit can produce more than one directory event, so this is a lower bound.
const evictions = stderr.split("\n").filter(line => line.includes("Removing file:")).length;
expect(evictions).toBeGreaterThanOrEqual(edits);
expect(stderr).toContain("LeakSanitizer: checking for leaks");
// LSan prints one blank-line-separated block per leaking allocation stack
// (each naming its allocation site) and then fails the exit.
const leaks = stderr.split(/\n\s*\n/).filter(block => /^(?:Direct|Indirect) leak of /.test(block));
expect(leaks).toEqual([]);
expect({ exitCode, signalCode: proc.signalCode }).toEqual({ exitCode: 0, signalCode: null });
});
});
1 change: 0 additions & 1 deletion test/no-validate-leaksan.txt
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,6 @@ test/bake/dev/server-sourcemap.test.ts
# Watcher Thread
test/bake/dev-and-prod.test.ts
test/bake/dev/bundle.test.ts
test/bake/dev/css.test.ts
test/bake/dev/esm.test.ts
test/bake/dev/hot.test.ts
test/bake/dev/react-spa.test.ts
Expand Down
Loading