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
56 changes: 50 additions & 6 deletions src/collections/multi_array_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -990,29 +990,33 @@ 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) {
/// retain list ordering. Returns the removed element, like [`pop`](Self::pop).
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) {
/// after it to preserve order. Returns the removed element, like [`pop`](Self::pop).
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 +1426,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 / shifts nothing.
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.
assert_eq!(list.ordered_remove(2), Foo { a: 3, b: 3, c: 3 });
assert_eq!(list.items::<"a", u32>(), &[0, 5]);
}

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

// Under Miri this also checks that every `name` is freed exactly once.
#[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
4 changes: 3 additions & 1 deletion src/watcher/Watcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,8 @@ impl Watcher {
if item == last_item || self.watchlist.len() <= item as usize {
continue;
}
self.watchlist.swap_remove(item as usize);
// Frees an owned `file_path`; the fd was closed in the first pass.
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 +1052,7 @@ impl fmt::Display for Op {
// ─── WatchItem ────────────────────────────────────────────────────────────

pub struct WatchItem {
/// Freed by `flush_evictions` when `Owned`; borrowed bytes must outlive the entry.
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 @@ if (globalThis.reloaded++ >= ${maxCount}) process.exit(0);
},
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 () => {
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