From 82678235ab6a417b6f239984a66d210582a8578b Mon Sep 17 00:00:00 2001 From: Lucien Greathouse Date: Wed, 16 Oct 2019 15:45:23 -0700 Subject: [PATCH] VFS Improvements (#259) This PR refactors all of the methods on `Vfs` from accepting `&mut self` to accepting `&self` and keeping data wrapped in a mutex. This builds on previous changes to make reference count file contents and cleans up the last places where we're returning borrowed data out of the VFS interface. Once this change lands, there are two possible directions we can go that I see: * Conservative: Refactor all remaining `&mut Vfs` handles to `&Vfs` * Interesting: Embrace ref counting by changing `Vfs` methods to accept `self: Arc`, which makes the `VfsEntry` API no longer need an explicit `Vfs` argument for its operations. * Change VfsFetcher to be immutable with internal locking * Refactor Vfs::would_be_resident * Refactor Vfs::read_if_not_exists * Refactor Vfs::raise_file_removed * Refactor Vfs::raise_file_changed * Add Vfs::get_internal as bits of Vfs::get * Switch Vfs to use internal locking * Migrate all Vfs methods from &mut self to &self * Make VfsEntry access Vfs immutably * Remove outer VFS locking (#260) * Refactor all snapshot middleware to accept &Vfs instead of &mut Vfs * Remove outer VFS Mutex across the board --- src/change_processor.rs | 14 +- src/common_setup.rs | 2 +- src/serve_session.rs | 16 +- src/snapshot_middleware/csv.rs | 2 +- src/snapshot_middleware/dir.rs | 2 +- src/snapshot_middleware/json_model.rs | 2 +- src/snapshot_middleware/lua.rs | 6 +- src/snapshot_middleware/middleware.rs | 2 +- src/snapshot_middleware/mod.rs | 2 +- src/snapshot_middleware/project.rs | 4 +- src/snapshot_middleware/rbxlx.rs | 2 +- src/snapshot_middleware/rbxm.rs | 2 +- src/snapshot_middleware/rbxmx.rs | 2 +- src/snapshot_middleware/txt.rs | 2 +- src/snapshot_middleware/user_plugins.rs | 2 +- src/vfs/fetcher.rs | 19 +- src/vfs/noop_fetcher.rs | 16 +- src/vfs/real_fetcher.rs | 53 ++-- src/vfs/test_fetcher.rs | 16 +- src/vfs/vfs.rs | 380 +++++++++++++----------- src/web/ui.rs | 4 +- 21 files changed, 292 insertions(+), 258 deletions(-) diff --git a/src/change_processor.rs b/src/change_processor.rs index 55238ce8..5e0d3220 100644 --- a/src/change_processor.rs +++ b/src/change_processor.rs @@ -21,10 +21,10 @@ pub struct ChangeProcessor { } impl ChangeProcessor { - pub fn start( + pub fn start( tree: Arc>, message_queue: Arc>, - vfs: Arc>>, + vfs: Arc>, ) -> Self { let (shutdown_sender, shutdown_receiver) = crossbeam_channel::bounded(1); @@ -47,12 +47,9 @@ impl ChangeProcessor { shutdown_receiver: Receiver<()>, tree: Arc>, message_queue: Arc>, - vfs: Arc>>, + vfs: Arc>, ) { - let vfs_receiver = { - let vfs = vfs.lock().unwrap(); - vfs.change_receiver() - }; + let vfs_receiver = vfs.change_receiver(); // Crossbeam's select macro generates code that Clippy doesn't like, and // Clippy blames us for it. @@ -65,7 +62,6 @@ impl ChangeProcessor { log::trace!("Vfs event: {:?}", event); let applied_patches = { - let mut vfs = vfs.lock().unwrap(); vfs.commit_change(&event).expect("Error applying VFS change"); let mut tree = tree.lock().unwrap(); @@ -102,7 +98,7 @@ impl ChangeProcessor { // TODO: Use persisted snapshot // context struct instead of // recreating it every time. - let snapshot = snapshot_from_vfs(&mut InstanceSnapshotContext::default(), &mut vfs, &entry) + let snapshot = snapshot_from_vfs(&mut InstanceSnapshotContext::default(), &vfs, &entry) .expect("snapshot failed") .expect("snapshot did not return an instance"); diff --git a/src/common_setup.rs b/src/common_setup.rs index 50734117..51914181 100644 --- a/src/common_setup.rs +++ b/src/common_setup.rs @@ -14,7 +14,7 @@ use crate::{ pub fn start( fuzzy_project_path: &Path, - vfs: &mut Vfs, + vfs: &Vfs, ) -> (Option, RojoTree) { log::trace!("Loading project file from {}", fuzzy_project_path.display()); let maybe_project = match Project::load_fuzzy(fuzzy_project_path) { diff --git a/src/serve_session.rs b/src/serve_session.rs index 6a24bf65..cd3865ce 100644 --- a/src/serve_session.rs +++ b/src/serve_session.rs @@ -52,7 +52,7 @@ pub struct ServeSession { /// /// The main use for accessing it from the session is for debugging issues /// with Rojo's live-sync protocol. - vfs: Arc>>, + vfs: Arc>, /// A queue of changes that have been applied to `tree` that affect clients. /// @@ -71,7 +71,7 @@ pub struct ServeSession { /// Methods that need thread-safety bounds on VfsFetcher are limited to this /// block to prevent needing to spread Send + Sync + 'static into everything /// that handles ServeSession. -impl ServeSession { +impl ServeSession { /// Start a new serve session from the given in-memory filesystem and start /// path. /// @@ -92,7 +92,7 @@ impl ServeSession { let tree = Arc::new(Mutex::new(tree)); let message_queue = Arc::new(message_queue); - let vfs = Arc::new(Mutex::new(vfs)); + let vfs = Arc::new(vfs); log::trace!("Starting ChangeProcessor"); let change_processor = ChangeProcessor::start( @@ -122,8 +122,8 @@ impl ServeSession { self.tree.lock().unwrap() } - pub fn vfs(&self) -> MutexGuard<'_, Vfs> { - self.vfs.lock().unwrap() + pub fn vfs(&self) -> &Vfs { + &self.vfs } pub fn message_queue(&self) -> &MessageQueue { @@ -179,7 +179,7 @@ mod serve_session { #[test] fn just_folder() { - let mut vfs = Vfs::new(NoopFetcher); + let vfs = Vfs::new(NoopFetcher); vfs.debug_load_snapshot("/foo", VfsSnapshot::empty_dir()); @@ -191,7 +191,7 @@ mod serve_session { #[test] fn project_with_folder() { - let mut vfs = Vfs::new(NoopFetcher); + let vfs = Vfs::new(NoopFetcher); vfs.debug_load_snapshot( "/foo", @@ -218,7 +218,7 @@ mod serve_session { #[test] fn script_with_meta() { - let mut vfs = Vfs::new(NoopFetcher); + let vfs = Vfs::new(NoopFetcher); vfs.debug_load_snapshot( "/root", diff --git a/src/snapshot_middleware/csv.rs b/src/snapshot_middleware/csv.rs index 08c2b755..c9a09ef2 100644 --- a/src/snapshot_middleware/csv.rs +++ b/src/snapshot_middleware/csv.rs @@ -21,7 +21,7 @@ pub struct SnapshotCsv; impl SnapshotMiddleware for SnapshotCsv { fn from_vfs( _context: &mut InstanceSnapshotContext, - vfs: &mut Vfs, + vfs: &Vfs, entry: &VfsEntry, ) -> SnapshotInstanceResult<'static> { if entry.is_directory() { diff --git a/src/snapshot_middleware/dir.rs b/src/snapshot_middleware/dir.rs index 9b54ef59..7d1bed84 100644 --- a/src/snapshot_middleware/dir.rs +++ b/src/snapshot_middleware/dir.rs @@ -20,7 +20,7 @@ pub struct SnapshotDir; impl SnapshotMiddleware for SnapshotDir { fn from_vfs( context: &mut InstanceSnapshotContext, - vfs: &mut Vfs, + vfs: &Vfs, entry: &VfsEntry, ) -> SnapshotInstanceResult<'static> { if entry.is_file() { diff --git a/src/snapshot_middleware/json_model.rs b/src/snapshot_middleware/json_model.rs index 3ccde84c..18aaa53d 100644 --- a/src/snapshot_middleware/json_model.rs +++ b/src/snapshot_middleware/json_model.rs @@ -20,7 +20,7 @@ pub struct SnapshotJsonModel; impl SnapshotMiddleware for SnapshotJsonModel { fn from_vfs( _context: &mut InstanceSnapshotContext, - vfs: &mut Vfs, + vfs: &Vfs, entry: &VfsEntry, ) -> SnapshotInstanceResult<'static> { if entry.is_directory() { diff --git a/src/snapshot_middleware/lua.rs b/src/snapshot_middleware/lua.rs index bd6c54b2..2ebb7c2a 100644 --- a/src/snapshot_middleware/lua.rs +++ b/src/snapshot_middleware/lua.rs @@ -21,7 +21,7 @@ pub struct SnapshotLua; impl SnapshotMiddleware for SnapshotLua { fn from_vfs( context: &mut InstanceSnapshotContext, - vfs: &mut Vfs, + vfs: &Vfs, entry: &VfsEntry, ) -> SnapshotInstanceResult<'static> { let file_name = entry.path().file_name().unwrap().to_string_lossy(); @@ -54,7 +54,7 @@ impl SnapshotMiddleware for SnapshotLua { /// Core routine for turning Lua files into snapshots. fn snapshot_lua_file( - vfs: &mut Vfs, + vfs: &Vfs, entry: &VfsEntry, ) -> SnapshotInstanceResult<'static> { let file_name = entry.path().file_name().unwrap().to_string_lossy(); @@ -117,7 +117,7 @@ fn snapshot_lua_file( /// their parents, which acts similarly to `__init__.py` from the Python world. fn snapshot_init( context: &mut InstanceSnapshotContext, - vfs: &mut Vfs, + vfs: &Vfs, folder_entry: &VfsEntry, init_name: &str, ) -> SnapshotInstanceResult<'static> { diff --git a/src/snapshot_middleware/middleware.rs b/src/snapshot_middleware/middleware.rs index fe9a2ebb..6282e474 100644 --- a/src/snapshot_middleware/middleware.rs +++ b/src/snapshot_middleware/middleware.rs @@ -15,7 +15,7 @@ pub type SnapshotFileResult = Option<(String, VfsSnapshot)>; pub trait SnapshotMiddleware { fn from_vfs( context: &mut InstanceSnapshotContext, - vfs: &mut Vfs, + vfs: &Vfs, entry: &VfsEntry, ) -> SnapshotInstanceResult<'static>; diff --git a/src/snapshot_middleware/mod.rs b/src/snapshot_middleware/mod.rs index 4e466d7b..25f0bedb 100644 --- a/src/snapshot_middleware/mod.rs +++ b/src/snapshot_middleware/mod.rs @@ -44,7 +44,7 @@ macro_rules! middlewares { /// Generates a snapshot of instances from the given VfsEntry. pub fn snapshot_from_vfs( context: &mut InstanceSnapshotContext, - vfs: &mut Vfs, + vfs: &Vfs, entry: &VfsEntry, ) -> SnapshotInstanceResult<'static> { $( diff --git a/src/snapshot_middleware/project.rs b/src/snapshot_middleware/project.rs index a3050ff0..bb4a8ffe 100644 --- a/src/snapshot_middleware/project.rs +++ b/src/snapshot_middleware/project.rs @@ -23,7 +23,7 @@ pub struct SnapshotProject; impl SnapshotMiddleware for SnapshotProject { fn from_vfs( context: &mut InstanceSnapshotContext, - vfs: &mut Vfs, + vfs: &Vfs, entry: &VfsEntry, ) -> SnapshotInstanceResult<'static> { if entry.is_directory() { @@ -80,7 +80,7 @@ fn snapshot_project_node( context: &mut InstanceSnapshotContext, instance_name: &str, node: &ProjectNode, - vfs: &mut Vfs, + vfs: &Vfs, ) -> SnapshotInstanceResult<'static> { let name = Cow::Owned(instance_name.to_owned()); let mut class_name = node diff --git a/src/snapshot_middleware/rbxlx.rs b/src/snapshot_middleware/rbxlx.rs index 631afe22..2935fad4 100644 --- a/src/snapshot_middleware/rbxlx.rs +++ b/src/snapshot_middleware/rbxlx.rs @@ -16,7 +16,7 @@ pub struct SnapshotRbxlx; impl SnapshotMiddleware for SnapshotRbxlx { fn from_vfs( _context: &mut InstanceSnapshotContext, - vfs: &mut Vfs, + vfs: &Vfs, entry: &VfsEntry, ) -> SnapshotInstanceResult<'static> { if entry.is_directory() { diff --git a/src/snapshot_middleware/rbxm.rs b/src/snapshot_middleware/rbxm.rs index 8af33296..ec9b4ab1 100644 --- a/src/snapshot_middleware/rbxm.rs +++ b/src/snapshot_middleware/rbxm.rs @@ -18,7 +18,7 @@ pub struct SnapshotRbxm; impl SnapshotMiddleware for SnapshotRbxm { fn from_vfs( _context: &mut InstanceSnapshotContext, - vfs: &mut Vfs, + vfs: &Vfs, entry: &VfsEntry, ) -> SnapshotInstanceResult<'static> { if entry.is_directory() { diff --git a/src/snapshot_middleware/rbxmx.rs b/src/snapshot_middleware/rbxmx.rs index bd54160d..e7b05752 100644 --- a/src/snapshot_middleware/rbxmx.rs +++ b/src/snapshot_middleware/rbxmx.rs @@ -16,7 +16,7 @@ pub struct SnapshotRbxmx; impl SnapshotMiddleware for SnapshotRbxmx { fn from_vfs( _context: &mut InstanceSnapshotContext, - vfs: &mut Vfs, + vfs: &Vfs, entry: &VfsEntry, ) -> SnapshotInstanceResult<'static> { if entry.is_directory() { diff --git a/src/snapshot_middleware/txt.rs b/src/snapshot_middleware/txt.rs index 0b79eaac..54b08aa6 100644 --- a/src/snapshot_middleware/txt.rs +++ b/src/snapshot_middleware/txt.rs @@ -21,7 +21,7 @@ pub struct SnapshotTxt; impl SnapshotMiddleware for SnapshotTxt { fn from_vfs( _context: &mut InstanceSnapshotContext, - vfs: &mut Vfs, + vfs: &Vfs, entry: &VfsEntry, ) -> SnapshotInstanceResult<'static> { if entry.is_directory() { diff --git a/src/snapshot_middleware/user_plugins.rs b/src/snapshot_middleware/user_plugins.rs index f39d9336..10daecd4 100644 --- a/src/snapshot_middleware/user_plugins.rs +++ b/src/snapshot_middleware/user_plugins.rs @@ -16,7 +16,7 @@ pub struct SnapshotUserPlugins; impl SnapshotMiddleware for SnapshotUserPlugins { fn from_vfs( context: &mut InstanceSnapshotContext, - _vfs: &mut Vfs, + _vfs: &Vfs, _entry: &VfsEntry, ) -> SnapshotInstanceResult<'static> { // User plugins are only enabled if present on the snapshot context. diff --git a/src/vfs/fetcher.rs b/src/vfs/fetcher.rs index bb321093..7d38e370 100644 --- a/src/vfs/fetcher.rs +++ b/src/vfs/fetcher.rs @@ -17,20 +17,21 @@ pub enum FileType { /// In tests, it's stubbed out to do different versions of absolutely nothing /// depending on the test. pub trait VfsFetcher { - fn file_type(&mut self, path: &Path) -> io::Result; - fn read_children(&mut self, path: &Path) -> io::Result>; - fn read_contents(&mut self, path: &Path) -> io::Result>; + fn file_type(&self, path: &Path) -> io::Result; + fn read_children(&self, path: &Path) -> io::Result>; + fn read_contents(&self, path: &Path) -> io::Result>; - fn create_directory(&mut self, path: &Path) -> io::Result<()>; - fn write_file(&mut self, path: &Path, contents: &[u8]) -> io::Result<()>; - fn remove(&mut self, path: &Path) -> io::Result<()>; + fn create_directory(&self, path: &Path) -> io::Result<()>; + fn write_file(&self, path: &Path, contents: &[u8]) -> io::Result<()>; + fn remove(&self, path: &Path) -> io::Result<()>; - fn watch(&mut self, path: &Path); - fn unwatch(&mut self, path: &Path); fn receiver(&self) -> Receiver; + fn watch(&self, _path: &Path) {} + fn unwatch(&self, _path: &Path) {} + /// A method intended for debugging what paths the fetcher is watching. - fn watched_paths(&self) -> Vec<&Path> { + fn watched_paths(&self) -> Vec { Vec::new() } } diff --git a/src/vfs/noop_fetcher.rs b/src/vfs/noop_fetcher.rs index b0312185..58533a83 100644 --- a/src/vfs/noop_fetcher.rs +++ b/src/vfs/noop_fetcher.rs @@ -19,42 +19,42 @@ use super::{ pub struct NoopFetcher; impl VfsFetcher for NoopFetcher { - fn file_type(&mut self, _path: &Path) -> io::Result { + fn file_type(&self, _path: &Path) -> io::Result { Err(io::Error::new( io::ErrorKind::NotFound, "NoopFetcher always returns NotFound", )) } - fn read_children(&mut self, _path: &Path) -> io::Result> { + fn read_children(&self, _path: &Path) -> io::Result> { Err(io::Error::new( io::ErrorKind::NotFound, "NoopFetcher always returns NotFound", )) } - fn read_contents(&mut self, _path: &Path) -> io::Result> { + fn read_contents(&self, _path: &Path) -> io::Result> { Err(io::Error::new( io::ErrorKind::NotFound, "NoopFetcher always returns NotFound", )) } - fn create_directory(&mut self, _path: &Path) -> io::Result<()> { + fn create_directory(&self, _path: &Path) -> io::Result<()> { Ok(()) } - fn write_file(&mut self, _path: &Path, _contents: &[u8]) -> io::Result<()> { + fn write_file(&self, _path: &Path, _contents: &[u8]) -> io::Result<()> { Ok(()) } - fn remove(&mut self, _path: &Path) -> io::Result<()> { + fn remove(&self, _path: &Path) -> io::Result<()> { Ok(()) } - fn watch(&mut self, _path: &Path) {} + fn watch(&self, _path: &Path) {} - fn unwatch(&mut self, _path: &Path) {} + fn unwatch(&self, _path: &Path) {} fn receiver(&self) -> Receiver { crossbeam_channel::never() diff --git a/src/vfs/real_fetcher.rs b/src/vfs/real_fetcher.rs index f2f686d9..5dce585c 100644 --- a/src/vfs/real_fetcher.rs +++ b/src/vfs/real_fetcher.rs @@ -5,7 +5,7 @@ use std::{ collections::HashSet, fs, io, path::{Path, PathBuf}, - sync::mpsc, + sync::{mpsc, Mutex}, time::Duration, }; @@ -34,7 +34,7 @@ pub struct RealFetcher { // // `watcher` must be dropped before `_converter_thread` or else joining the // thread will cause a deadlock. - watcher: Option, + watcher: Option>, /// Thread handle to convert notify's mpsc channel messages into /// crossbeam_channel messages. @@ -45,7 +45,7 @@ pub struct RealFetcher { /// All of the paths that the fetcher is watching, tracked here because /// notify does not expose this information. - watched_paths: HashSet, + watched_paths: Mutex>, } impl RealFetcher { @@ -68,10 +68,12 @@ impl RealFetcher { // causing our program to deadlock. Once this is fixed, watcher no // longer needs to be optional, but is still maybe useful? let watcher = match watch_mode { - WatchMode::Enabled => Some( - notify::watcher(notify_sender, Duration::from_millis(300)) - .expect("Couldn't start 'notify' file watcher"), - ), + WatchMode::Enabled => { + let watcher = notify::watcher(notify_sender, Duration::from_millis(300)) + .expect("Couldn't start 'notify' file watcher"); + + Some(Mutex::new(watcher)) + } WatchMode::Disabled => None, }; @@ -79,7 +81,7 @@ impl RealFetcher { watcher, _converter_thread: handle, receiver, - watched_paths: HashSet::new(), + watched_paths: Mutex::new(HashSet::new()), } } } @@ -122,7 +124,7 @@ fn converter_thread(notify_receiver: mpsc::Receiver, sender: Sen } impl VfsFetcher for RealFetcher { - fn file_type(&mut self, path: &Path) -> io::Result { + fn file_type(&self, path: &Path) -> io::Result { let metadata = fs::metadata(path)?; if metadata.is_file() { @@ -132,7 +134,7 @@ impl VfsFetcher for RealFetcher { } } - fn read_children(&mut self, path: &Path) -> io::Result> { + fn read_children(&self, path: &Path) -> io::Result> { log::trace!("Reading directory {}", path.display()); let mut result = Vec::new(); @@ -146,25 +148,25 @@ impl VfsFetcher for RealFetcher { Ok(result) } - fn read_contents(&mut self, path: &Path) -> io::Result> { + fn read_contents(&self, path: &Path) -> io::Result> { log::trace!("Reading file {}", path.display()); fs::read(path) } - fn create_directory(&mut self, path: &Path) -> io::Result<()> { + fn create_directory(&self, path: &Path) -> io::Result<()> { log::trace!("Creating directory {}", path.display()); fs::create_dir(path) } - fn write_file(&mut self, path: &Path, contents: &[u8]) -> io::Result<()> { + fn write_file(&self, path: &Path, contents: &[u8]) -> io::Result<()> { log::trace!("Writing path {}", path.display()); fs::write(path, contents) } - fn remove(&mut self, path: &Path) -> io::Result<()> { + fn remove(&self, path: &Path) -> io::Result<()> { log::trace!("Removing path {}", path.display()); let metadata = fs::metadata(path)?; @@ -176,13 +178,16 @@ impl VfsFetcher for RealFetcher { } } - fn watch(&mut self, path: &Path) { + fn watch(&self, path: &Path) { log::trace!("Watching path {}", path.display()); - if let Some(watcher) = self.watcher.as_mut() { + if let Some(watcher_handle) = &self.watcher { + let mut watcher = watcher_handle.lock().unwrap(); + match watcher.watch(path, RecursiveMode::NonRecursive) { Ok(_) => { - self.watched_paths.insert(path.to_path_buf()); + let mut watched_paths = self.watched_paths.lock().unwrap(); + watched_paths.insert(path.to_path_buf()); } Err(err) => { log::warn!("Couldn't watch path {}: {:?}", path.display(), err); @@ -191,14 +196,17 @@ impl VfsFetcher for RealFetcher { } } - fn unwatch(&mut self, path: &Path) { + fn unwatch(&self, path: &Path) { log::trace!("Stopped watching path {}", path.display()); - if let Some(watcher) = self.watcher.as_mut() { + if let Some(watcher_handle) = &self.watcher { + let mut watcher = watcher_handle.lock().unwrap(); + // Remove the path from our watched paths regardless of the outcome // of notify's unwatch to ensure we drop old paths in the event of a // rename. - self.watched_paths.remove(path); + let mut watched_paths = self.watched_paths.lock().unwrap(); + watched_paths.remove(path); if let Err(err) = watcher.unwatch(path) { log::warn!("Couldn't unwatch path {}: {:?}", path.display(), err); @@ -210,7 +218,8 @@ impl VfsFetcher for RealFetcher { self.receiver.clone() } - fn watched_paths(&self) -> Vec<&Path> { - self.watched_paths.iter().map(|v| v.as_path()).collect() + fn watched_paths(&self) -> Vec { + let watched_paths = self.watched_paths.lock().unwrap(); + watched_paths.iter().map(|v| v.clone()).collect() } } diff --git a/src/vfs/test_fetcher.rs b/src/vfs/test_fetcher.rs index 9e37717c..e97f9334 100644 --- a/src/vfs/test_fetcher.rs +++ b/src/vfs/test_fetcher.rs @@ -107,7 +107,7 @@ impl TestFetcher { } impl VfsFetcher for TestFetcher { - fn file_type(&mut self, path: &Path) -> io::Result { + fn file_type(&self, path: &Path) -> io::Result { let inner = self.state.inner.lock().unwrap(); match inner.entries.get(path) { @@ -117,7 +117,7 @@ impl VfsFetcher for TestFetcher { } } - fn read_children(&mut self, path: &Path) -> io::Result> { + fn read_children(&self, path: &Path) -> io::Result> { let inner = self.state.inner.lock().unwrap(); Ok(inner @@ -129,7 +129,7 @@ impl VfsFetcher for TestFetcher { .collect()) } - fn read_contents(&mut self, path: &Path) -> io::Result> { + fn read_contents(&self, path: &Path) -> io::Result> { let inner = self.state.inner.lock().unwrap(); let node = inner.entries.get(path); @@ -144,31 +144,27 @@ impl VfsFetcher for TestFetcher { } } - fn create_directory(&mut self, _path: &Path) -> io::Result<()> { + fn create_directory(&self, _path: &Path) -> io::Result<()> { Err(io::Error::new( io::ErrorKind::Other, "TestFetcher is not mutable yet", )) } - fn write_file(&mut self, _path: &Path, _contents: &[u8]) -> io::Result<()> { + fn write_file(&self, _path: &Path, _contents: &[u8]) -> io::Result<()> { Err(io::Error::new( io::ErrorKind::Other, "TestFetcher is not mutable yet", )) } - fn remove(&mut self, _path: &Path) -> io::Result<()> { + fn remove(&self, _path: &Path) -> io::Result<()> { Err(io::Error::new( io::ErrorKind::Other, "TestFetcher is not mutable yet", )) } - fn watch(&mut self, _path: &Path) {} - - fn unwatch(&mut self, _path: &Path) {} - fn receiver(&self) -> Receiver { self.receiver.clone() } diff --git a/src/vfs/vfs.rs b/src/vfs/vfs.rs index 509e1e2e..ee3ee177 100644 --- a/src/vfs/vfs.rs +++ b/src/vfs/vfs.rs @@ -1,7 +1,7 @@ use std::{ io, path::{Path, PathBuf}, - sync::Arc, + sync::{Arc, Mutex}, }; use crossbeam_channel::Receiver; @@ -28,7 +28,7 @@ use super::{ pub struct Vfs { /// A hierarchical map from paths to items that have been read or partially /// read into memory by the Vfs. - data: PathMap, + data: Mutex>, /// This Vfs's fetcher, which is used for all actual interactions with the /// filesystem. It's referred to by the type parameter `F` all over, and is @@ -39,7 +39,7 @@ pub struct Vfs { impl Vfs { pub fn new(fetcher: F) -> Self { Self { - data: PathMap::new(), + data: Mutex::new(PathMap::new()), fetcher, } } @@ -48,108 +48,37 @@ impl Vfs { self.fetcher.receiver() } - pub fn commit_change(&mut self, event: &VfsEvent) -> FsResult<()> { + pub fn commit_change(&self, event: &VfsEvent) -> FsResult<()> { use VfsEvent::*; log::trace!("Committing Vfs change {:?}", event); + let mut data = self.data.lock().unwrap(); + match event { Created(path) | Modified(path) => { - self.raise_file_changed(path)?; + Self::raise_file_changed(&mut data, &self.fetcher, path)?; } Removed(path) => { - self.raise_file_removed(path)?; + Self::raise_file_removed(&mut data, &self.fetcher, path)?; } } Ok(()) } - fn raise_file_changed(&mut self, path: impl AsRef) -> FsResult<()> { - let path = path.as_ref(); - - if !self.would_be_resident(path) { - return Ok(()); - } - - let new_type = self - .fetcher - .file_type(path) - .map_err(|err| FsError::new(err, path.to_path_buf()))?; - - match self.data.get_mut(path) { - Some(existing_item) => { - match (existing_item, &new_type) { - (VfsItem::File(existing_file), FileType::File) => { - // Invalidate the existing file contents. - // We can probably be smarter about this by reading the changed file. - existing_file.contents = None; - } - (VfsItem::Directory(_), FileType::Directory) => { - // No changes required, a directory updating doesn't mean anything to us. - self.fetcher.watch(path); - } - (VfsItem::File(_), FileType::Directory) => { - self.data.remove(path); - self.data.insert( - path.to_path_buf(), - VfsItem::new_from_type(FileType::Directory, path), - ); - self.fetcher.watch(path); - } - (VfsItem::Directory(_), FileType::File) => { - self.data.remove(path); - self.data.insert( - path.to_path_buf(), - VfsItem::new_from_type(FileType::File, path), - ); - self.fetcher.unwatch(path); - } - } - } - None => { - self.data - .insert(path.to_path_buf(), VfsItem::new_from_type(new_type, path)); - } - } - - Ok(()) + pub fn get(&self, path: impl AsRef) -> FsResult { + let mut data = self.data.lock().unwrap(); + Self::get_internal(&mut data, &self.fetcher, path) } - fn raise_file_removed(&mut self, path: impl AsRef) -> FsResult<()> { + pub fn get_contents(&self, path: impl AsRef) -> FsResult>> { let path = path.as_ref(); - if !self.would_be_resident(path) { - return Ok(()); - } + let mut data = self.data.lock().unwrap(); + Self::read_if_not_exists(&mut data, &self.fetcher, path)?; - self.data.remove(path); - self.fetcher.unwatch(path); - Ok(()) - } - - pub fn get(&mut self, path: impl AsRef) -> FsResult { - self.read_if_not_exists(path.as_ref())?; - - let item = self.data.get(path.as_ref()).unwrap(); - - let is_file = match item { - VfsItem::File(_) => true, - VfsItem::Directory(_) => false, - }; - - Ok(VfsEntry { - path: item.path().to_path_buf(), - is_file, - }) - } - - pub fn get_contents(&mut self, path: impl AsRef) -> FsResult>> { - let path = path.as_ref(); - - self.read_if_not_exists(path)?; - - match self.data.get_mut(path).unwrap() { + match data.get_mut(path).unwrap() { VfsItem::File(file) => { if file.contents.is_none() { file.contents = Some( @@ -169,26 +98,26 @@ impl Vfs { } } - pub fn get_children(&mut self, path: impl AsRef) -> FsResult> { + pub fn get_children(&self, path: impl AsRef) -> FsResult> { let path = path.as_ref(); - self.read_if_not_exists(path)?; + let mut data = self.data.lock().unwrap(); + Self::read_if_not_exists(&mut data, &self.fetcher, path)?; - match self.data.get_mut(path).unwrap() { + match data.get_mut(path).unwrap() { VfsItem::Directory(dir) => { self.fetcher.watch(path); let enumerated = dir.children_enumerated; if enumerated { - self.data - .children(path) + data.children(path) .unwrap() // TODO: Handle None here, which means the PathMap entry did not exist. .into_iter() .map(PathBuf::from) // Convert paths from &Path to PathBuf .collect::>() // Collect all PathBufs, since self.get needs to borrow self mutably. .into_iter() - .map(|path| self.get(path)) + .map(|path| Self::get_internal(&mut data, &self.fetcher, path)) .collect::>>() } else { dir.children_enumerated = true; @@ -197,7 +126,7 @@ impl Vfs { .read_children(path) .map_err(|err| FsError::new(err, path.to_path_buf()))? .into_iter() - .map(|path| self.get(path)) + .map(|path| Self::get_internal(&mut data, &self.fetcher, path)) .collect::>>() } } @@ -208,6 +137,118 @@ impl Vfs { } } + fn get_internal( + data: &mut PathMap, + fetcher: &F, + path: impl AsRef, + ) -> FsResult { + let path = path.as_ref(); + + Self::read_if_not_exists(data, fetcher, path)?; + + let item = data.get(path).unwrap(); + + let is_file = match item { + VfsItem::File(_) => true, + VfsItem::Directory(_) => false, + }; + + Ok(VfsEntry { + path: item.path().to_path_buf(), + is_file, + }) + } + + fn raise_file_changed( + data: &mut PathMap, + fetcher: &F, + path: impl AsRef, + ) -> FsResult<()> { + let path = path.as_ref(); + + if !Self::would_be_resident(&data, path) { + return Ok(()); + } + + let new_type = fetcher + .file_type(path) + .map_err(|err| FsError::new(err, path.to_path_buf()))?; + + match data.get_mut(path) { + Some(existing_item) => { + match (existing_item, &new_type) { + (VfsItem::File(existing_file), FileType::File) => { + // Invalidate the existing file contents. + // We can probably be smarter about this by reading the changed file. + existing_file.contents = None; + } + (VfsItem::Directory(_), FileType::Directory) => { + // No changes required, a directory updating doesn't mean anything to us. + fetcher.watch(path); + } + (VfsItem::File(_), FileType::Directory) => { + data.remove(path); + data.insert( + path.to_path_buf(), + VfsItem::new_from_type(FileType::Directory, path), + ); + fetcher.watch(path); + } + (VfsItem::Directory(_), FileType::File) => { + data.remove(path); + data.insert( + path.to_path_buf(), + VfsItem::new_from_type(FileType::File, path), + ); + fetcher.unwatch(path); + } + } + } + None => { + data.insert(path.to_path_buf(), VfsItem::new_from_type(new_type, path)); + } + } + + Ok(()) + } + + fn raise_file_removed( + data: &mut PathMap, + fetcher: &F, + path: impl AsRef, + ) -> FsResult<()> { + let path = path.as_ref(); + + if !Self::would_be_resident(data, path) { + return Ok(()); + } + + data.remove(path); + fetcher.unwatch(path); + Ok(()) + } + + /// Attempts to read the path into the `Vfs` if it doesn't exist. + /// + /// This does not necessitate that file contents or directory children will + /// be read. Depending on the `VfsFetcher` implementation that the `Vfs` + /// is using, this call may read exactly only the given path and no more. + fn read_if_not_exists(data: &mut PathMap, fetcher: &F, path: &Path) -> FsResult<()> { + if !data.contains_key(path) { + let kind = fetcher + .file_type(path) + .map_err(|err| FsError::new(err, path.to_path_buf()))?; + + if kind == FileType::Directory { + fetcher.watch(path); + } + + data.insert(path.to_path_buf(), VfsItem::new_from_type(kind, path)); + } + + Ok(()) + } + /// Tells whether the given path, if it were loaded, would be loaded if it /// existed. /// @@ -218,113 +259,108 @@ impl Vfs { /// tangible changes to the in-memory filesystem. If a path would be /// resident, we need to read it, and if its contents were known before, we /// need to update them. - fn would_be_resident(&self, path: &Path) -> bool { - if self.data.contains_key(path) { + fn would_be_resident(data: &PathMap, path: &Path) -> bool { + if data.contains_key(path) { return true; } if let Some(parent) = path.parent() { - if let Some(VfsItem::Directory(dir)) = self.data.get(parent) { + if let Some(VfsItem::Directory(dir)) = data.get(parent) { return !dir.children_enumerated; } } false } - - /// Attempts to read the path into the `Vfs` if it doesn't exist. - /// - /// This does not necessitate that file contents or directory children will - /// be read. Depending on the `VfsFetcher` implementation that the `Vfs` - /// is using, this call may read exactly only the given path and no more. - fn read_if_not_exists(&mut self, path: &Path) -> FsResult<()> { - if !self.data.contains_key(path) { - let kind = self - .fetcher - .file_type(path) - .map_err(|err| FsError::new(err, path.to_path_buf()))?; - - if kind == FileType::Directory { - self.fetcher.watch(path); - } - - self.data - .insert(path.to_path_buf(), VfsItem::new_from_type(kind, path)); - } - - Ok(()) - } } /// Contains extra methods that should only be used for debugging. They're /// broken out into a separate trait to make it more explicit to depend on them. pub trait VfsDebug { - fn debug_load_snapshot>(&mut self, path: P, snapshot: VfsSnapshot); + fn debug_load_snapshot>(&self, path: P, snapshot: VfsSnapshot); fn debug_is_file(&self, path: &Path) -> bool; - fn debug_contents<'a>(&'a self, path: &Path) -> Option<&'a [u8]>; - fn debug_children<'a>(&'a self, path: &Path) -> Option<(bool, Vec<&'a Path>)>; - fn debug_orphans(&self) -> Vec<&Path>; - fn debug_watched_paths(&self) -> Vec<&Path>; + fn debug_contents(&self, path: &Path) -> Option>>; + fn debug_children(&self, path: &Path) -> Option<(bool, Vec)>; + fn debug_orphans(&self) -> Vec; + fn debug_watched_paths(&self) -> Vec; } impl VfsDebug for Vfs { - fn debug_load_snapshot>(&mut self, path: P, snapshot: VfsSnapshot) { - let path = path.as_ref(); + fn debug_load_snapshot>(&self, path: P, snapshot: VfsSnapshot) { + fn load_snapshot>( + data: &mut PathMap, + path: P, + snapshot: VfsSnapshot, + ) { + let path = path.as_ref(); - match snapshot { - VfsSnapshot::File(file) => { - self.data.insert( - path.to_path_buf(), - VfsItem::File(VfsFile { - path: path.to_path_buf(), - contents: Some(Arc::new(file.contents)), - }), - ); - } - VfsSnapshot::Directory(directory) => { - self.data.insert( - path.to_path_buf(), - VfsItem::Directory(VfsDirectory { - path: path.to_path_buf(), - children_enumerated: true, - }), - ); + match snapshot { + VfsSnapshot::File(file) => { + data.insert( + path.to_path_buf(), + VfsItem::File(VfsFile { + path: path.to_path_buf(), + contents: Some(Arc::new(file.contents)), + }), + ); + } + VfsSnapshot::Directory(directory) => { + data.insert( + path.to_path_buf(), + VfsItem::Directory(VfsDirectory { + path: path.to_path_buf(), + children_enumerated: true, + }), + ); - for (child_name, child) in directory.children.into_iter() { - self.debug_load_snapshot(path.join(child_name), child); + for (child_name, child) in directory.children.into_iter() { + load_snapshot(data, path.join(child_name), child); + } } } } + + let mut data = self.data.lock().unwrap(); + load_snapshot(&mut data, path, snapshot) } fn debug_is_file(&self, path: &Path) -> bool { - match self.data.get(path) { + let data = self.data.lock().unwrap(); + match data.get(path) { Some(VfsItem::File(_)) => true, _ => false, } } - fn debug_contents<'a>(&'a self, path: &Path) -> Option<&'a [u8]> { - match self.data.get(path) { - Some(VfsItem::File(file)) => file.contents.as_ref().map(|vec| vec.as_slice()), + fn debug_contents(&self, path: &Path) -> Option>> { + let data = self.data.lock().unwrap(); + match data.get(path) { + Some(VfsItem::File(file)) => file.contents.clone(), _ => None, } } - fn debug_children<'a>(&'a self, path: &Path) -> Option<(bool, Vec<&'a Path>)> { - match self.data.get(path) { - Some(VfsItem::Directory(dir)) => { - Some((dir.children_enumerated, self.data.children(path).unwrap())) - } + fn debug_children(&self, path: &Path) -> Option<(bool, Vec)> { + let data = self.data.lock().unwrap(); + match data.get(path) { + Some(VfsItem::Directory(dir)) => Some(( + dir.children_enumerated, + data.children(path) + .unwrap() + .iter() + .map(|path| path.to_path_buf()) + .collect(), + )), _ => None, } } - fn debug_orphans(&self) -> Vec<&Path> { - self.data.orphans().collect() + fn debug_orphans(&self) -> Vec { + let data = self.data.lock().unwrap(); + data.orphans().map(|path| path.to_path_buf()).collect() } - fn debug_watched_paths(&self) -> Vec<&Path> { + fn debug_watched_paths(&self) -> Vec { self.fetcher.watched_paths() } } @@ -345,11 +381,11 @@ impl VfsEntry { &self.path } - pub fn contents<'vfs>(&self, vfs: &'vfs mut Vfs) -> FsResult>> { + pub fn contents(&self, vfs: &Vfs) -> FsResult>> { vfs.get_contents(&self.path) } - pub fn children(&self, vfs: &mut Vfs) -> FsResult> { + pub fn children(&self, vfs: &Vfs) -> FsResult> { vfs.get_children(&self.path) } @@ -414,7 +450,7 @@ mod test { #[test] fn from_snapshot_file() { - let mut vfs = Vfs::new(NoopFetcher); + let vfs = Vfs::new(NoopFetcher); let file = VfsSnapshot::file("hello, world!"); vfs.debug_load_snapshot("/hello.txt", file); @@ -425,7 +461,7 @@ mod test { #[test] fn from_snapshot_dir() { - let mut vfs = Vfs::new(NoopFetcher); + let vfs = Vfs::new(NoopFetcher); let dir = VfsSnapshot::dir(hashmap! { "a.txt" => VfsSnapshot::file("contents of a.txt"), "b.lua" => VfsSnapshot::file("contents of b.lua"), @@ -470,7 +506,7 @@ mod test { } impl VfsFetcher for MockFetcher { - fn file_type(&mut self, path: &Path) -> io::Result { + fn file_type(&self, path: &Path) -> io::Result { if path == Path::new("/dir/a.txt") { return Ok(FileType::File); } @@ -478,7 +514,7 @@ mod test { unimplemented!(); } - fn read_contents(&mut self, path: &Path) -> io::Result> { + fn read_contents(&self, path: &Path) -> io::Result> { if path == Path::new("/dir/a.txt") { let inner = self.inner.borrow(); @@ -488,26 +524,22 @@ mod test { unimplemented!(); } - fn read_children(&mut self, _path: &Path) -> io::Result> { + fn read_children(&self, _path: &Path) -> io::Result> { unimplemented!(); } - fn create_directory(&mut self, _path: &Path) -> io::Result<()> { + fn create_directory(&self, _path: &Path) -> io::Result<()> { unimplemented!(); } - fn write_file(&mut self, _path: &Path, _contents: &[u8]) -> io::Result<()> { + fn write_file(&self, _path: &Path, _contents: &[u8]) -> io::Result<()> { unimplemented!(); } - fn remove(&mut self, _path: &Path) -> io::Result<()> { + fn remove(&self, _path: &Path) -> io::Result<()> { unimplemented!(); } - fn watch(&mut self, _path: &Path) {} - - fn unwatch(&mut self, _path: &Path) {} - fn receiver(&self) -> Receiver { crossbeam_channel::never() } @@ -532,7 +564,7 @@ mod test { mock_state.a_contents = "Changed contents"; } - vfs.raise_file_changed("/dir/a.txt") + vfs.commit_change(&VfsEvent::Modified(PathBuf::from("/dir/a.txt"))) .expect("error processing file change"); let contents = a.contents(&mut vfs).expect("mock file contents error"); @@ -555,7 +587,7 @@ mod test { assert_eq!(contents.as_slice(), b"hello, world!"); - vfs.raise_file_removed("/hello.txt") + vfs.commit_change(&VfsEvent::Removed(PathBuf::from("/hello.txt"))) .expect("error processing file removal"); match vfs.get("hello.txt") { diff --git a/src/web/ui.rs b/src/web/ui.rs index a4224948..8f7276be 100644 --- a/src/web/ui.rs +++ b/src/web/ui.rs @@ -102,7 +102,7 @@ impl UiService { let orphans: Vec<_> = vfs .debug_orphans() .into_iter() - .map(|path| Self::render_vfs_path(&vfs, path, true)) + .map(|path| Self::render_vfs_path(&vfs, &path, true)) .collect(); let watched_list: Vec<_> = vfs @@ -153,7 +153,7 @@ impl UiService { let children: Vec<_> = children .into_iter() - .map(|child| Self::render_vfs_path(vfs, child, false)) + .map(|child| Self::render_vfs_path(vfs, &child, false)) .collect(); let note = if is_exhaustive {