From 5ecc9606af10d7ad400c344e7944e97dd3d99886 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Mon, 3 Apr 2023 18:15:07 -0700 Subject: [PATCH] Use synchronous locks in FakeFs This way, the state can be accessed without running the deterministic executor. --- .../src/tests/randomized_integration_tests.rs | 21 +- crates/fs/src/fs.rs | 297 ++++++++---------- crates/project/src/worktree.rs | 2 +- 3 files changed, 149 insertions(+), 171 deletions(-) diff --git a/crates/collab/src/tests/randomized_integration_tests.rs b/crates/collab/src/tests/randomized_integration_tests.rs index 44e1891363..583271b342 100644 --- a/crates/collab/src/tests/randomized_integration_tests.rs +++ b/crates/collab/src/tests/randomized_integration_tests.rs @@ -624,7 +624,7 @@ async fn apply_client_operation( ); ensure_project_shared(&project, client, cx).await; - if !client.fs.paths().await.contains(&new_root_path) { + if !client.fs.paths().contains(&new_root_path) { client.fs.create_dir(&new_root_path).await.unwrap(); } project @@ -1350,7 +1350,6 @@ impl TestPlan { return None; } - let executor = cx.background(); self.operation_ix += 1; let call = cx.read(ActiveCall::global); Some(loop { @@ -1467,7 +1466,7 @@ impl TestPlan { .choose(&mut self.rng) .cloned() else { continue }; let project_root_name = root_name_for_project(&project, cx); - let mut paths = executor.block(client.fs.paths()); + let mut paths = client.fs.paths(); paths.remove(0); let new_root_path = if paths.is_empty() || self.rng.gen() { Path::new("/").join(&self.next_root_dir_name(user_id)) @@ -1637,14 +1636,16 @@ impl TestPlan { // Update a git index 91..=95 => { - let repo_path = executor - .block(client.fs.directories()) + let repo_path = client + .fs + .directories() .choose(&mut self.rng) .unwrap() .clone(); - let mut file_paths = executor - .block(client.fs.files()) + let mut file_paths = client + .fs + .files() .into_iter() .filter(|path| path.starts_with(&repo_path)) .collect::>(); @@ -1673,7 +1674,7 @@ impl TestPlan { let is_dir = self.rng.gen::(); let content; let mut path; - let dir_paths = cx.background().block(client.fs.directories()); + let dir_paths = client.fs.directories(); if is_dir { content = String::new(); @@ -1683,7 +1684,7 @@ impl TestPlan { content = Alphanumeric.sample_string(&mut self.rng, 16); // Create a new file or overwrite an existing file - let file_paths = cx.background().block(client.fs.files()); + let file_paths = client.fs.files(); if file_paths.is_empty() || self.rng.gen_bool(0.5) { path = dir_paths.choose(&mut self.rng).unwrap().clone(); path.push(gen_file_name(&mut self.rng)); @@ -1789,7 +1790,7 @@ async fn simulate_client( let fs = fs.clone(); let plan = plan.clone(); async move { - let files = fs.files().await; + let files = fs.files(); let count = plan.lock().rng.gen_range::(1..3); let files = (0..count) .map(|_| files.choose(&mut plan.lock().rng).unwrap()) diff --git a/crates/fs/src/fs.rs b/crates/fs/src/fs.rs index fd713ef3b5..4d0b0c4f44 100644 --- a/crates/fs/src/fs.rs +++ b/crates/fs/src/fs.rs @@ -5,7 +5,7 @@ use fsevent::EventStream; use futures::{future::BoxFuture, Stream, StreamExt}; use git2::Repository as LibGitRepository; use lazy_static::lazy_static; -use parking_lot::Mutex as SyncMutex; +use parking_lot::Mutex; use regex::Regex; use repository::GitRepository; use rope::Rope; @@ -27,8 +27,6 @@ use util::ResultExt; #[cfg(any(test, feature = "test-support"))] use collections::{btree_map, BTreeMap}; #[cfg(any(test, feature = "test-support"))] -use futures::lock::Mutex; -#[cfg(any(test, feature = "test-support"))] use repository::FakeGitRepositoryState; #[cfg(any(test, feature = "test-support"))] use std::sync::Weak; @@ -117,7 +115,7 @@ pub trait Fs: Send + Sync { path: &Path, latency: Duration, ) -> Pin>>>; - fn open_repo(&self, abs_dot_git: &Path) -> Option>>; + fn open_repo(&self, abs_dot_git: &Path) -> Option>>; fn is_fake(&self) -> bool; #[cfg(any(test, feature = "test-support"))] fn as_fake(&self) -> &FakeFs; @@ -350,11 +348,11 @@ impl Fs for RealFs { }))) } - fn open_repo(&self, dotgit_path: &Path) -> Option>> { + fn open_repo(&self, dotgit_path: &Path) -> Option>> { LibGitRepository::open(&dotgit_path) .log_err() - .and_then::>, _>(|libgit_repository| { - Some(Arc::new(SyncMutex::new(libgit_repository))) + .and_then::>, _>(|libgit_repository| { + Some(Arc::new(Mutex::new(libgit_repository))) }) } @@ -396,7 +394,7 @@ enum FakeFsEntry { inode: u64, mtime: SystemTime, entries: BTreeMap>>, - git_repo_state: Option>>, + git_repo_state: Option>>, }, Symlink { target: PathBuf, @@ -405,18 +403,14 @@ enum FakeFsEntry { #[cfg(any(test, feature = "test-support"))] impl FakeFsState { - async fn read_path<'a>(&'a self, target: &Path) -> Result>> { + fn read_path<'a>(&'a self, target: &Path) -> Result>> { Ok(self .try_read_path(target) - .await .ok_or_else(|| anyhow!("path does not exist: {}", target.display()))? .0) } - async fn try_read_path<'a>( - &'a self, - target: &Path, - ) -> Option<(Arc>, PathBuf)> { + fn try_read_path<'a>(&'a self, target: &Path) -> Option<(Arc>, PathBuf)> { let mut path = target.to_path_buf(); let mut real_path = PathBuf::new(); let mut entry_stack = Vec::new(); @@ -438,10 +432,10 @@ impl FakeFsState { } Component::Normal(name) => { let current_entry = entry_stack.last().cloned()?; - let current_entry = current_entry.lock().await; + let current_entry = current_entry.lock(); if let FakeFsEntry::Dir { entries, .. } = &*current_entry { let entry = entries.get(name.to_str().unwrap()).cloned()?; - let _entry = entry.lock().await; + let _entry = entry.lock(); if let FakeFsEntry::Symlink { target, .. } = &*_entry { let mut target = target.clone(); target.extend(path_components); @@ -462,7 +456,7 @@ impl FakeFsState { entry_stack.pop().map(|entry| (entry, real_path)) } - async fn write_path(&self, path: &Path, callback: Fn) -> Result + fn write_path(&self, path: &Path, callback: Fn) -> Result where Fn: FnOnce(btree_map::Entry>>) -> Result, { @@ -472,8 +466,8 @@ impl FakeFsState { .ok_or_else(|| anyhow!("cannot overwrite the root"))?; let parent_path = path.parent().unwrap(); - let parent = self.read_path(parent_path).await?; - let mut parent = parent.lock().await; + let parent = self.read_path(parent_path)?; + let mut parent = parent.lock(); let new_entry = parent .dir_entries(parent_path)? .entry(filename.to_str().unwrap().into()); @@ -529,7 +523,7 @@ impl FakeFs { } pub async fn insert_file(&self, path: impl AsRef, content: String) { - let mut state = self.state.lock().await; + let mut state = self.state.lock(); let path = path.as_ref(); let inode = state.next_inode; let mtime = state.next_mtime; @@ -552,13 +546,12 @@ impl FakeFs { } Ok(()) }) - .await .unwrap(); state.emit_event(&[path]); } pub async fn insert_symlink(&self, path: impl AsRef, target: PathBuf) { - let mut state = self.state.lock().await; + let mut state = self.state.lock(); let path = path.as_ref(); let file = Arc::new(Mutex::new(FakeFsEntry::Symlink { target })); state @@ -572,21 +565,20 @@ impl FakeFs { Ok(()) } }) - .await .unwrap(); state.emit_event(&[path]); } pub async fn pause_events(&self) { - self.state.lock().await.events_paused = true; + self.state.lock().events_paused = true; } pub async fn buffered_event_count(&self) -> usize { - self.state.lock().await.buffered_events.len() + self.state.lock().buffered_events.len() } pub async fn flush_events(&self, count: usize) { - self.state.lock().await.flush_events(count); + self.state.lock().flush_events(count); } #[must_use] @@ -625,9 +617,9 @@ impl FakeFs { } pub async fn set_index_for_repo(&self, dot_git: &Path, head_state: &[(&Path, String)]) { - let mut state = self.state.lock().await; - let entry = state.read_path(dot_git).await.unwrap(); - let mut entry = entry.lock().await; + let mut state = self.state.lock(); + let entry = state.read_path(dot_git).unwrap(); + let mut entry = entry.lock(); if let FakeFsEntry::Dir { git_repo_state, .. } = &mut *entry { let repo_state = git_repo_state.get_or_insert_with(Default::default); @@ -646,12 +638,12 @@ impl FakeFs { } } - pub async fn paths(&self) -> Vec { + pub fn paths(&self) -> Vec { let mut result = Vec::new(); let mut queue = collections::VecDeque::new(); - queue.push_back((PathBuf::from("/"), self.state.lock().await.root.clone())); + queue.push_back((PathBuf::from("/"), self.state.lock().root.clone())); while let Some((path, entry)) = queue.pop_front() { - if let FakeFsEntry::Dir { entries, .. } = &*entry.lock().await { + if let FakeFsEntry::Dir { entries, .. } = &*entry.lock() { for (name, entry) in entries { queue.push_back((path.join(name), entry.clone())); } @@ -661,12 +653,12 @@ impl FakeFs { result } - pub async fn directories(&self) -> Vec { + pub fn directories(&self) -> Vec { let mut result = Vec::new(); let mut queue = collections::VecDeque::new(); - queue.push_back((PathBuf::from("/"), self.state.lock().await.root.clone())); + queue.push_back((PathBuf::from("/"), self.state.lock().root.clone())); while let Some((path, entry)) = queue.pop_front() { - if let FakeFsEntry::Dir { entries, .. } = &*entry.lock().await { + if let FakeFsEntry::Dir { entries, .. } = &*entry.lock() { for (name, entry) in entries { queue.push_back((path.join(name), entry.clone())); } @@ -676,12 +668,12 @@ impl FakeFs { result } - pub async fn files(&self) -> Vec { + pub fn files(&self) -> Vec { let mut result = Vec::new(); let mut queue = collections::VecDeque::new(); - queue.push_back((PathBuf::from("/"), self.state.lock().await.root.clone())); + queue.push_back((PathBuf::from("/"), self.state.lock().root.clone())); while let Some((path, entry)) = queue.pop_front() { - let e = entry.lock().await; + let e = entry.lock(); match &*e { FakeFsEntry::File { .. } => result.push(path), FakeFsEntry::Dir { entries, .. } => { @@ -745,11 +737,11 @@ impl FakeFsEntry { impl Fs for FakeFs { async fn create_dir(&self, path: &Path) -> Result<()> { self.simulate_random_delay().await; - let mut state = self.state.lock().await; let mut created_dirs = Vec::new(); let mut cur_path = PathBuf::new(); for component in path.components() { + let mut state = self.state.lock(); cur_path.push(component); if cur_path == Path::new("/") { continue; @@ -759,29 +751,27 @@ impl Fs for FakeFs { let mtime = state.next_mtime; state.next_mtime += Duration::from_nanos(1); state.next_inode += 1; - state - .write_path(&cur_path, |entry| { - entry.or_insert_with(|| { - created_dirs.push(cur_path.clone()); - Arc::new(Mutex::new(FakeFsEntry::Dir { - inode, - mtime, - entries: Default::default(), - git_repo_state: None, - })) - }); - Ok(()) - }) - .await?; + state.write_path(&cur_path, |entry| { + entry.or_insert_with(|| { + created_dirs.push(cur_path.clone()); + Arc::new(Mutex::new(FakeFsEntry::Dir { + inode, + mtime, + entries: Default::default(), + git_repo_state: None, + })) + }); + Ok(()) + })? } - state.emit_event(&created_dirs); + self.state.lock().emit_event(&created_dirs); Ok(()) } async fn create_file(&self, path: &Path, options: CreateOptions) -> Result<()> { self.simulate_random_delay().await; - let mut state = self.state.lock().await; + let mut state = self.state.lock(); let inode = state.next_inode; let mtime = state.next_mtime; state.next_mtime += Duration::from_nanos(1); @@ -791,23 +781,21 @@ impl Fs for FakeFs { mtime, content: String::new(), })); - state - .write_path(path, |entry| { - match entry { - btree_map::Entry::Occupied(mut e) => { - if options.overwrite { - *e.get_mut() = file; - } else if !options.ignore_if_exists { - return Err(anyhow!("path already exists: {}", path.display())); - } - } - btree_map::Entry::Vacant(e) => { - e.insert(file); + state.write_path(path, |entry| { + match entry { + btree_map::Entry::Occupied(mut e) => { + if options.overwrite { + *e.get_mut() = file; + } else if !options.ignore_if_exists { + return Err(anyhow!("path already exists: {}", path.display())); } } - Ok(()) - }) - .await?; + btree_map::Entry::Vacant(e) => { + e.insert(file); + } + } + Ok(()) + })?; state.emit_event(&[path]); Ok(()) } @@ -815,33 +803,29 @@ impl Fs for FakeFs { async fn rename(&self, old_path: &Path, new_path: &Path, options: RenameOptions) -> Result<()> { let old_path = normalize_path(old_path); let new_path = normalize_path(new_path); - let mut state = self.state.lock().await; - let moved_entry = state - .write_path(&old_path, |e| { - if let btree_map::Entry::Occupied(e) = e { - Ok(e.remove()) - } else { - Err(anyhow!("path does not exist: {}", &old_path.display())) - } - }) - .await?; - state - .write_path(&new_path, |e| { - match e { - btree_map::Entry::Occupied(mut e) => { - if options.overwrite { - *e.get_mut() = moved_entry; - } else if !options.ignore_if_exists { - return Err(anyhow!("path already exists: {}", new_path.display())); - } - } - btree_map::Entry::Vacant(e) => { - e.insert(moved_entry); + let mut state = self.state.lock(); + let moved_entry = state.write_path(&old_path, |e| { + if let btree_map::Entry::Occupied(e) = e { + Ok(e.remove()) + } else { + Err(anyhow!("path does not exist: {}", &old_path.display())) + } + })?; + state.write_path(&new_path, |e| { + match e { + btree_map::Entry::Occupied(mut e) => { + if options.overwrite { + *e.get_mut() = moved_entry; + } else if !options.ignore_if_exists { + return Err(anyhow!("path already exists: {}", new_path.display())); } } - Ok(()) - }) - .await?; + btree_map::Entry::Vacant(e) => { + e.insert(moved_entry); + } + } + Ok(()) + })?; state.emit_event(&[old_path, new_path]); Ok(()) } @@ -849,35 +833,33 @@ impl Fs for FakeFs { async fn copy_file(&self, source: &Path, target: &Path, options: CopyOptions) -> Result<()> { let source = normalize_path(source); let target = normalize_path(target); - let mut state = self.state.lock().await; + let mut state = self.state.lock(); let mtime = state.next_mtime; let inode = util::post_inc(&mut state.next_inode); state.next_mtime += Duration::from_nanos(1); - let source_entry = state.read_path(&source).await?; - let content = source_entry.lock().await.file_content(&source)?.clone(); - let entry = state - .write_path(&target, |e| match e { - btree_map::Entry::Occupied(e) => { - if options.overwrite { - Ok(Some(e.get().clone())) - } else if !options.ignore_if_exists { - return Err(anyhow!("{target:?} already exists")); - } else { - Ok(None) - } + let source_entry = state.read_path(&source)?; + let content = source_entry.lock().file_content(&source)?.clone(); + let entry = state.write_path(&target, |e| match e { + btree_map::Entry::Occupied(e) => { + if options.overwrite { + Ok(Some(e.get().clone())) + } else if !options.ignore_if_exists { + return Err(anyhow!("{target:?} already exists")); + } else { + Ok(None) } - btree_map::Entry::Vacant(e) => Ok(Some( - e.insert(Arc::new(Mutex::new(FakeFsEntry::File { - inode, - mtime, - content: String::new(), - }))) - .clone(), - )), - }) - .await?; + } + btree_map::Entry::Vacant(e) => Ok(Some( + e.insert(Arc::new(Mutex::new(FakeFsEntry::File { + inode, + mtime, + content: String::new(), + }))) + .clone(), + )), + })?; if let Some(entry) = entry { - entry.lock().await.set_file_content(&target, content)?; + entry.lock().set_file_content(&target, content)?; } state.emit_event(&[target]); Ok(()) @@ -890,9 +872,9 @@ impl Fs for FakeFs { .ok_or_else(|| anyhow!("cannot remove the root"))?; let base_name = path.file_name().unwrap(); - let mut state = self.state.lock().await; - let parent_entry = state.read_path(parent_path).await?; - let mut parent_entry = parent_entry.lock().await; + let mut state = self.state.lock(); + let parent_entry = state.read_path(parent_path)?; + let mut parent_entry = parent_entry.lock(); let entry = parent_entry .dir_entries(parent_path)? .entry(base_name.to_str().unwrap().into()); @@ -905,7 +887,7 @@ impl Fs for FakeFs { } btree_map::Entry::Occupied(e) => { { - let mut entry = e.get().lock().await; + let mut entry = e.get().lock(); let children = entry.dir_entries(&path)?; if !options.recursive && !children.is_empty() { return Err(anyhow!("{path:?} is not empty")); @@ -924,9 +906,9 @@ impl Fs for FakeFs { .parent() .ok_or_else(|| anyhow!("cannot remove the root"))?; let base_name = path.file_name().unwrap(); - let mut state = self.state.lock().await; - let parent_entry = state.read_path(parent_path).await?; - let mut parent_entry = parent_entry.lock().await; + let mut state = self.state.lock(); + let parent_entry = state.read_path(parent_path)?; + let mut parent_entry = parent_entry.lock(); let entry = parent_entry .dir_entries(parent_path)? .entry(base_name.to_str().unwrap().into()); @@ -937,7 +919,7 @@ impl Fs for FakeFs { } } btree_map::Entry::Occupied(e) => { - e.get().lock().await.file_content(&path)?; + e.get().lock().file_content(&path)?; e.remove(); } } @@ -953,9 +935,9 @@ impl Fs for FakeFs { async fn load(&self, path: &Path) -> Result { let path = normalize_path(path); self.simulate_random_delay().await; - let state = self.state.lock().await; - let entry = state.read_path(&path).await?; - let entry = entry.lock().await; + let state = self.state.lock(); + let entry = state.read_path(&path)?; + let entry = entry.lock(); entry.file_content(&path).cloned() } @@ -978,8 +960,8 @@ impl Fs for FakeFs { async fn canonicalize(&self, path: &Path) -> Result { let path = normalize_path(path); self.simulate_random_delay().await; - let state = self.state.lock().await; - if let Some((_, real_path)) = state.try_read_path(&path).await { + let state = self.state.lock(); + if let Some((_, real_path)) = state.try_read_path(&path) { Ok(real_path) } else { Err(anyhow!("path does not exist: {}", path.display())) @@ -989,9 +971,9 @@ impl Fs for FakeFs { async fn is_file(&self, path: &Path) -> bool { let path = normalize_path(path); self.simulate_random_delay().await; - let state = self.state.lock().await; - if let Some((entry, _)) = state.try_read_path(&path).await { - entry.lock().await.is_file() + let state = self.state.lock(); + if let Some((entry, _)) = state.try_read_path(&path) { + entry.lock().is_file() } else { false } @@ -1000,9 +982,9 @@ impl Fs for FakeFs { async fn metadata(&self, path: &Path) -> Result> { self.simulate_random_delay().await; let path = normalize_path(path); - let state = self.state.lock().await; - if let Some((entry, real_path)) = state.try_read_path(&path).await { - let entry = entry.lock().await; + let state = self.state.lock(); + if let Some((entry, real_path)) = state.try_read_path(&path) { + let entry = entry.lock(); let is_symlink = real_path != path; Ok(Some(match &*entry { @@ -1031,9 +1013,9 @@ impl Fs for FakeFs { ) -> Result>>>> { self.simulate_random_delay().await; let path = normalize_path(path); - let state = self.state.lock().await; - let entry = state.read_path(&path).await?; - let mut entry = entry.lock().await; + let state = self.state.lock(); + let entry = state.read_path(&path)?; + let mut entry = entry.lock(); let children = entry.dir_entries(&path)?; let paths = children .keys() @@ -1047,10 +1029,9 @@ impl Fs for FakeFs { path: &Path, _: Duration, ) -> Pin>>> { - let mut state = self.state.lock().await; self.simulate_random_delay().await; let (tx, rx) = smol::channel::unbounded(); - state.event_txs.push(tx); + self.state.lock().event_txs.push(tx); let path = path.to_path_buf(); let executor = self.executor.clone(); Box::pin(futures::StreamExt::filter(rx, move |events| { @@ -1065,22 +1046,18 @@ impl Fs for FakeFs { })) } - fn open_repo(&self, abs_dot_git: &Path) -> Option>> { - smol::block_on(async move { - let state = self.state.lock().await; - let entry = state.read_path(abs_dot_git).await.unwrap(); - let mut entry = entry.lock().await; - if let FakeFsEntry::Dir { git_repo_state, .. } = &mut *entry { - let state = git_repo_state - .get_or_insert_with(|| { - Arc::new(SyncMutex::new(FakeGitRepositoryState::default())) - }) - .clone(); - Some(repository::FakeGitRepository::open(state)) - } else { - None - } - }) + fn open_repo(&self, abs_dot_git: &Path) -> Option>> { + let state = self.state.lock(); + let entry = state.read_path(abs_dot_git).unwrap(); + let mut entry = entry.lock(); + if let FakeFsEntry::Dir { git_repo_state, .. } = &mut *entry { + let state = git_repo_state + .get_or_insert_with(|| Arc::new(Mutex::new(FakeGitRepositoryState::default()))) + .clone(); + Some(repository::FakeGitRepository::open(state)) + } else { + None + } } fn is_fake(&self) -> bool { @@ -1213,7 +1190,7 @@ mod tests { .await; assert_eq!( - fs.files().await, + fs.files(), vec![ PathBuf::from("/root/dir1/a"), PathBuf::from("/root/dir1/b"), diff --git a/crates/project/src/worktree.rs b/crates/project/src/worktree.rs index 2357052d2c..b1aebf29f1 100644 --- a/crates/project/src/worktree.rs +++ b/crates/project/src/worktree.rs @@ -3729,7 +3729,7 @@ mod tests { ) { let mut files = Vec::new(); let mut dirs = Vec::new(); - for path in fs.as_fake().paths().await { + for path in fs.as_fake().paths() { if path.starts_with(root_path) { if fs.is_file(&path).await { files.push(path);