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 scripts/rust-miri.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ const MIRI_CRATES = [
"bun_shell_parser",
"bun_threading",
"bun_url",
"bun_watcher",
"bun_wyhash",
];

Expand Down
345 changes: 96 additions & 249 deletions src/jsc/hot_reloader.rs

Large diffs are not rendered by default.

7 changes: 2 additions & 5 deletions src/resolver/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -903,9 +903,6 @@ pub mod fs {
}

impl EntriesOption {
// Payload is `&'static mut DirEntry`; auto-deref coerces to `&DirEntry` / `&mut DirEntry`.
bun_core::enum_unwrap!(pub EntriesOption, Entries => fn entries / entries_mut -> DirEntry);

pub(crate) fn as_entries(&self) -> Option<&DirEntry> {
match self {
EntriesOption::Entries(entries) => Some(&**entries),
Expand Down Expand Up @@ -1005,7 +1002,7 @@ pub mod fs {
// Cross-thread exclusion is provided by the mutex asserted above.
unsafe { &mut *entries_option_map() }
}
pub fn get(&mut self, key: &[u8]) -> Option<&mut EntriesOption> {
pub(crate) fn get(&mut self, key: &[u8]) -> Option<&mut EntriesOption> {
self.inner().get(key)
}
pub(crate) fn get_or_put(&mut self, key: &[u8]) -> crate::CrateResult<bun_alloc::Result> {
Expand Down Expand Up @@ -1052,7 +1049,7 @@ pub mod fs {
/// Port of `entries: *EntriesOption.Map`. The resolver body addresses
/// this directly (`rfs.entries.get_or_put(..)`); modeled as the wrapper
/// `EntriesMap` (bun_alloc has no BSSMap equivalent).
pub entries: EntriesMap,
pub(crate) entries: EntriesMap,
pub(crate) cwd: &'static [u8],
#[cfg(not(windows))]
pub(crate) file_limit: usize,
Expand Down
8 changes: 6 additions & 2 deletions src/resolver/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3462,7 +3462,10 @@ impl<'a> Resolver<'a> {
// outlives the resolver. Hoist the `&'static [u8] dir` read out so no `&EntriesOption`
// temporary is live when the raw `*mut` is passed below (avoids a needless Unique
// retag that would pop the shared tag mid-argument-list under Stacked Borrows).
let dir_entries_dir = unsafe { &*dir_entries_option }.entries().dir;
let dir_entries_dir = unsafe { &*dir_entries_option }
.as_entries()
.expect("unreachable")
.dir;
self.dir_info_uncached(
dir_info_ptr,
dir_entries_dir,
Expand Down Expand Up @@ -6084,7 +6087,8 @@ impl<'a> Resolver<'a> {
// `DirEntry` is a separate process-lifetime allocation, so the shared
// `BackRef` survives entries-map traffic. All uses below are `&self`
// reads under `entries_mutex`.
let dir_entries = bun_ptr::BackRef::new(unsafe { &*_entries }.entries());
let dir_entries =
bun_ptr::BackRef::new(unsafe { &*_entries }.as_entries().expect("unreachable"));
macro_rules! entries {
() => {
dir_entries.get()
Expand Down
168 changes: 152 additions & 16 deletions src/watcher/Watcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ pub type Event = WatchEvent;
pub type WatchList = MultiArrayList<WatchItem>;
pub type HashType = u32;
pub type WatchItemIndex = u16;
pub const MAX_EVICTION_COUNT: usize = 8096;

const NO_WATCH_ITEM: WatchItemIndex = WatchItemIndex::MAX;

Expand Down Expand Up @@ -122,8 +121,7 @@ pub struct Watcher {
/// thread) after the loop exits.
pub(crate) close_descriptors: bun_core::AtomicCell<bool>,

pub(crate) evict_list: [WatchItemIndex; MAX_EVICTION_COUNT],
pub(crate) evict_list_i: WatchItemIndex,
pub(crate) evict_list: Vec<WatchItemIndex>,

/// Scratch snapshot of `watchlist.eventlist_index` used by
/// `watch_loop_cycle`; owned by the watcher thread.
Expand Down Expand Up @@ -202,8 +200,7 @@ impl Watcher {
thread: None,
running: bun_core::AtomicCell::new(true),
close_descriptors: bun_core::AtomicCell::new(false),
evict_list: [0; MAX_EVICTION_COUNT],
evict_list_i: 0,
evict_list: Vec::new(),
#[cfg(any(target_os = "linux", target_os = "android"))]
eventlist_index_scratch: Vec::new(),
thread_lock: ThreadLock::init_unlocked(),
Expand Down Expand Up @@ -378,7 +375,7 @@ impl Watcher {
}

pub fn flush_evictions(&mut self) {
if self.evict_list_i == 0 {
if self.evict_list.is_empty() {
return;
}
// The close+swap_remove below must be serialized against the JS
Expand All @@ -397,20 +394,18 @@ impl Watcher {
self.mutex.is_held_by_current_thread(),
"flush_evictions: caller must hold self.mutex (platform watcher holds it around on_file_update)",
);
let evict_list_i = self.evict_list_i as usize;

// swapRemove messes up the order
// But, it only messes up the order if any elements in the list appear after the item being removed
// So if we just sort the list by the biggest index first, that should be fine
index_sort::sort_slice_by(&mut self.evict_list[0..evict_list_i], |a, b| b.cmp(a));
index_sort::sort_slice_by(&mut self.evict_list, |a, b| b.cmp(a));

// reshaped for borrowck — capture fds.len() before loop
let slice = self.watchlist.slice();
let fds = slice.items_fd();
let fds_len = fds.len();
let mut last_item = NO_WATCH_ITEM;

for &item in &self.evict_list[0..evict_list_i] {
for &item in &self.evict_list {
// catch duplicates, since the list is sorted, duplicates will appear right after each other
if item == last_item {
continue;
Expand All @@ -433,7 +428,7 @@ impl Watcher {

last_item = NO_WATCH_ITEM;
// This is split into two passes because reading the slice while modified is potentially unsafe.
for i in 0..evict_list_i {
for i in 0..self.evict_list.len() {
let item = self.evict_list[i];
if item == last_item || self.watchlist.len() <= item as usize {
continue;
Expand All @@ -458,7 +453,7 @@ impl Watcher {
last_item = item;
}

self.evict_list_i = 0;
self.evict_list.clear();
}

fn watch_loop(&mut self) -> sys::Result<()> {
Expand Down Expand Up @@ -557,6 +552,9 @@ impl Watcher {
// so we must copy into a NUL-terminated scratch buffer (mirrors the
// directory branch below) instead of pointing at the caller's slice.
let mut buf = bun_paths::path_buffer_pool::get();
if file_path.len() >= buf.len() {
return Err(sys::Error::new(sys::E::ENAMETOOLONG, sys::Tag::watch));
}
let slice: &ZStr = if CLONE_FILE_PATH {
buf[0..file_path.len()].copy_from_slice(file_path);
buf[file_path.len()] = 0;
Expand Down Expand Up @@ -631,6 +629,9 @@ impl Watcher {
#[cfg(any(target_os = "linux", target_os = "android"))]
let eventlist_index = {
let mut buf = bun_paths::path_buffer_pool::get();
if file_path.len() >= buf.len() {
return Err(sys::Error::new(sys::E::ENAMETOOLONG, sys::Tag::watch));
}
let path: &ZStr = if CLONE_FILE_PATH
&& !file_path.is_empty()
&& file_path[file_path.len() - 1] == 0
Expand Down Expand Up @@ -926,14 +927,12 @@ impl Watcher {

debug_assert!(index != NO_WATCH_ITEM);

self.evict_list[self.evict_list_i as usize] = index;
self.evict_list_i += 1;
self.evict_list.push(index);

if kind == WatchItemKind::Directory {
for &parent in parents {
if parent == hash {
self.evict_list[self.evict_list_i as usize] = parent as WatchItemIndex;
self.evict_list_i += 1;
self.evict_list.push(parent as WatchItemIndex);
}
}
}
Expand Down Expand Up @@ -965,6 +964,66 @@ impl Watcher {
}
}

// ─── directory events ─────────────────────────────────────────────────────

/// The `parent_hash` of the files in `dir`: the hash of `dir` with a trailing separator.
fn parent_hash_of(dir: &[u8], dir_hash: HashType) -> HashType {
if dir.last() == Some(&bun_paths::SEP) {
return dir_hash;
}
let mut hasher = bun_wyhash::Wyhash::init(0);
hasher.update(dir);
hasher.update(&[bun_paths::SEP]);
hasher.final_() as HashType
}

/// The `parent_hash` of the files in the folder at `folder_index`, then that of its other spellings.
pub fn folder_hashes(watchlist: &WatchList, folder_index: usize) -> (HashType, Vec<HashType>) {
let file_paths = watchlist.items_file_path();
let hashes = watchlist.items_hash();
let folder = parent_hash_of(&file_paths[folder_index], hashes[folder_index]);
// One inotify watch per inode: the first spelling of a folder gets its events.
#[cfg(any(target_os = "linux", target_os = "android"))]
let other_folders = {
let kinds = watchlist.items_kind();
let watch_descriptors = watchlist.items_eventlist_index();
let mut other_folders: Vec<HashType> = Vec::new();
for (other_index, other) in watch_descriptors.iter().enumerate() {
if *other != watch_descriptors[folder_index]
|| kinds[other_index] != WatchItemKind::Directory
{
continue;
}
let other_folder = parent_hash_of(&file_paths[other_index], hashes[other_index]);
if other_folder != folder && !other_folders.contains(&other_folder) {
other_folders.push(other_folder);
}
}
other_folders
};
#[cfg(not(any(target_os = "linux", target_os = "android")))]
let other_folders = Vec::new();
(folder, other_folders)
}

/// Whether a directory event that names `changed_names` names the file at `path`, whatever the ASCII letter case.
#[inline]
pub fn names_file(changed_names: &[ChangedFilePath], path: &[u8]) -> bool {
for changed_name in changed_names.iter().flatten() {
let changed_name = changed_name.as_bytes();
let Some(name_at) = path.len().checked_sub(changed_name.len()) else {
continue;
};
if name_at > 0
&& path[name_at - 1] == bun_paths::SEP
&& path[name_at..].eq_ignore_ascii_case(changed_name)
{
Comment thread
robobun marked this conversation as resolved.
return true;
}
}
false
}

// ─── WatchEvent ───────────────────────────────────────────────────────────

#[derive(Clone, Copy, Default)]
Expand Down Expand Up @@ -1139,3 +1198,80 @@ impl WatchItemColumns for bun_collections::multi_array_list::Slice<WatchItem> {
self.items::<"eventlist_index", platform::EventListIndex>()
}
}

#[cfg(test)]
mod tests {
use super::*;

fn item(path: &'static [u8], kind: WatchItemKind, watch_descriptor: i32) -> WatchItem {
let _ = watch_descriptor;
WatchItem {
file_path: Cow::Borrowed(path),
hash: Watcher::get_hash(path),
fd: Fd::INVALID,
count: 0,
parent_hash: Watcher::get_hash(
bun_paths::fs::PathName::init(path).dir_with_trailing_slash(),
),
kind,
package_json: None,
#[cfg(any(target_os = "linux", target_os = "android"))]
eventlist_index: watch_descriptor,
}
}

fn watchlist(items: Vec<WatchItem>) -> WatchList {
let mut list = WatchList::default();
for item in items {
list.append(item).unwrap();
}
list
}

#[test]
fn folder_hashes_is_the_parent_hash_of_the_files_in_the_folder() {
use WatchItemKind::{Directory, File};
let list = watchlist(vec![
item(b"/p/lib", Directory, 1),
item(b"/p/lib/a.ts", File, 2),
item(b"/p/src/", Directory, 3),
item(b"/p/src/b.ts", File, 4),
]);
let parents = list.items_parent_hash();
assert_eq!(folder_hashes(&list, 0), (parents[1], vec![]));
assert_eq!(folder_hashes(&list, 2), (parents[3], vec![]));
}

#[cfg(any(target_os = "linux", target_os = "android"))]
#[test]
fn folder_hashes_adds_the_other_spellings_that_share_the_watch_descriptor() {
use WatchItemKind::{Directory, File};
let list = watchlist(vec![
item(b"/p/link/", Directory, 1),
item(b"/p/link", Directory, 1),
item(b"/p/real/", Directory, 1),
item(b"/p/real/a.ts", File, 1),
item(b"/p/other/", Directory, 5),
item(b"/p/other/b.ts", File, 6),
]);
let parents = list.items_parent_hash();
let link = Watcher::get_hash(b"/p/link/");
assert_eq!(folder_hashes(&list, 0), (link, vec![parents[3]]));
Comment on lines +1248 to +1259

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit (optional): a maintainer running cargo test -p bun_watcher on Windows gets two failing unit tests that pass on Linux. names_file and parent_hash_of compare against bun_paths::SEP, which is \\ on Windows, but the ungated tests at src/watcher/Watcher.rs:1230 and src/watcher/Watcher.rs:1264 hard-code / paths. Fix: either gate the new mod tests (or these two cases) with #[cfg(not(windows))] like the existing descriptor test at Watcher.rs:1244, or build the fixture paths from SEP so every platform's native separator is used.

Why this was flagged

The new #[cfg(test)] mod tests in src/watcher/Watcher.rs:1202 is compiled on every platform; only folder_hashes_adds_the_other_spellings_that_share_the_watch_descriptor at Watcher.rs:1244 is gated to linux/android. names_file at src/watcher/Watcher.rs:1017 requires path[name_at - 1] == bun_paths::SEP, and parent_hash_of at src/watcher/Watcher.rs:970 checks dir.last() == Some(&bun_paths::SEP). bun_paths::SEP is \\ on Windows (SEP_WINDOWS at src/paths/lib.rs:65). The test names_file_compares_the_last_component_of_the_path at Watcher.rs:1264 asserts names_file(&names, b"/p/lib/b.ts") is true, which is false on Windows because byte 6 is /, not \\. The test folder_hashes_is_the_parent_hash_of_the_files_in_the_folder at Watcher.rs:1230 compares parent_hash_of(b"/p/lib", ..) with parent_hash built from dir_with_trailing_slash(), so it also fails there. CI only runs these on ubuntu (.github/workflows/rust-lints.yml:78), so the failure shows only for a developer running the crate's tests on Windows; the base branch had no unit tests in this crate.

Verification: nit — triggered when a maintainer runs cargo test -p bun_watcher on a Windows host; CI never does (.github/workflows/rust-lints.yml:76-78 is runs-on: ubuntu-latest). names_file requires path[name_at - 1] == bun_paths::SEP, so assert!(names_file(&names, b"/p/lib/b.ts")) at line 1268 fails. parent_hash_of appends bun_paths::SEP, so the assert_eq! at lines 1241-1242 fail.

assert_eq!(folder_hashes(&list, 1), (link, vec![parents[3]]));
assert_eq!(folder_hashes(&list, 2), (parents[3], vec![link]));
assert_eq!(folder_hashes(&list, 4), (parents[5], vec![]));
}

#[test]
fn names_file_compares_the_last_component_of_the_path() {
let names: [ChangedFilePath; 3] = [None, Some(zstr!("b.ts")), Some(zstr!("Page.TSX"))];
assert!(names_file(&names, b"/p/lib/b.ts"));
assert!(names_file(&names, b"/p/lib/page.tsx"));
assert!(!names_file(&names, b"/p/lib/ab.ts"));
assert!(!names_file(&names, b"/p/lib/b.tsx"));
assert!(!names_file(&names, b"/p/lib/b.ts/"));
assert!(!names_file(&names, b"b.ts"));
assert!(!names_file(&[], b"/p/lib/b.ts"));
assert!(!names_file(&[Some(zstr!(""))], b"/p/lib/b.ts"));
}
}
2 changes: 1 addition & 1 deletion src/watcher/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,5 @@ pub use WatchItemKind as Kind;
pub use watcher_impl::{
AnyResolveWatcher, ChangedFilePath, Event, FdOwnership, HashType, MAX_COUNT, Op, PackageJSON,
REQUIRES_FILE_DESCRIPTORS, WATCH_OPEN_FLAGS, WatchEvent, WatchItemColumns, WatchItemKind,
WatchList, Watcher, WatcherContext,
WatchList, Watcher, WatcherContext, folder_hashes, names_file,
};
Loading
Loading