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<Self>`, 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

@@ -21,10 +21,10 @@ pub struct ChangeProcessor {
 }
 
 impl ChangeProcessor {
-    pub fn start<F: VfsFetcher + Send + 'static>(
+    pub fn start<F: VfsFetcher + Send + Sync + 'static>(
         tree: Arc<Mutex<RojoTree>>,
         message_queue: Arc<MessageQueue<AppliedPatchSet>>,
-        vfs: Arc<Mutex<Vfs<F>>>,
+        vfs: Arc<Vfs<F>>,
     ) -> Self {
         let (shutdown_sender, shutdown_receiver) = crossbeam_channel::bounded(1);
 
@@ -47,12 +47,9 @@ impl ChangeProcessor {
         shutdown_receiver: Receiver<()>,
         tree: Arc<Mutex<RojoTree>>,
         message_queue: Arc<MessageQueue<AppliedPatchSet>>,
-        vfs: Arc<Mutex<Vfs<F>>>,
+        vfs: Arc<Vfs<F>>,
     ) {
-        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");
 

src/common_setup.rs

@@ -14,7 +14,7 @@ use crate::{
 
 pub fn start<F: VfsFetcher>(
     fuzzy_project_path: &Path,
-    vfs: &mut Vfs<F>,
+    vfs: &Vfs<F>,
 ) -> (Option<Project>, RojoTree) {
     log::trace!("Loading project file from {}", fuzzy_project_path.display());
     let maybe_project = match Project::load_fuzzy(fuzzy_project_path) {

src/serve_session.rs

@@ -52,7 +52,7 @@ pub struct ServeSession<F> {
     ///
     /// The main use for accessing it from the session is for debugging issues
     /// with Rojo's live-sync protocol.
-    vfs: Arc<Mutex<Vfs<F>>>,
+    vfs: Arc<Vfs<F>>,
 
     /// A queue of changes that have been applied to `tree` that affect clients.
     ///
@@ -71,7 +71,7 @@ pub struct ServeSession<F> {
 /// 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<F: VfsFetcher + Send + 'static> ServeSession<F> {
+impl<F: VfsFetcher + Send + Sync + 'static> ServeSession<F> {
     /// Start a new serve session from the given in-memory filesystem  and start
     /// path.
     ///
@@ -92,7 +92,7 @@ impl<F: VfsFetcher + Send + 'static> ServeSession<F> {
 
         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<F: VfsFetcher> ServeSession<F> {
         self.tree.lock().unwrap()
     }
 
-    pub fn vfs(&self) -> MutexGuard<'_, Vfs<F>> {
-        self.vfs.lock().unwrap()
+    pub fn vfs(&self) -> &Vfs<F> {
+        &self.vfs
     }
 
     pub fn message_queue(&self) -> &MessageQueue<AppliedPatchSet> {
@@ -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",

src/snapshot_middleware/csv.rs

@@ -21,7 +21,7 @@ pub struct SnapshotCsv;
 impl SnapshotMiddleware for SnapshotCsv {
     fn from_vfs<F: VfsFetcher>(
         _context: &mut InstanceSnapshotContext,
-        vfs: &mut Vfs<F>,
+        vfs: &Vfs<F>,
         entry: &VfsEntry,
     ) -> SnapshotInstanceResult<'static> {
         if entry.is_directory() {

src/snapshot_middleware/dir.rs

@@ -20,7 +20,7 @@ pub struct SnapshotDir;
 impl SnapshotMiddleware for SnapshotDir {
     fn from_vfs<F: VfsFetcher>(
         context: &mut InstanceSnapshotContext,
-        vfs: &mut Vfs<F>,
+        vfs: &Vfs<F>,
         entry: &VfsEntry,
     ) -> SnapshotInstanceResult<'static> {
         if entry.is_file() {

src/snapshot_middleware/json_model.rs

@@ -20,7 +20,7 @@ pub struct SnapshotJsonModel;
 impl SnapshotMiddleware for SnapshotJsonModel {
     fn from_vfs<F: VfsFetcher>(
         _context: &mut InstanceSnapshotContext,
-        vfs: &mut Vfs<F>,
+        vfs: &Vfs<F>,
         entry: &VfsEntry,
     ) -> SnapshotInstanceResult<'static> {
         if entry.is_directory() {

src/snapshot_middleware/lua.rs

@@ -21,7 +21,7 @@ pub struct SnapshotLua;
 impl SnapshotMiddleware for SnapshotLua {
     fn from_vfs<F: VfsFetcher>(
         context: &mut InstanceSnapshotContext,
-        vfs: &mut Vfs<F>,
+        vfs: &Vfs<F>,
         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<F: VfsFetcher>(
-    vfs: &mut Vfs<F>,
+    vfs: &Vfs<F>,
     entry: &VfsEntry,
 ) -> SnapshotInstanceResult<'static> {
     let file_name = entry.path().file_name().unwrap().to_string_lossy();
@@ -117,7 +117,7 @@ fn snapshot_lua_file<F: VfsFetcher>(
 /// their parents, which acts similarly to `__init__.py` from the Python world.
 fn snapshot_init<F: VfsFetcher>(
     context: &mut InstanceSnapshotContext,
-    vfs: &mut Vfs<F>,
+    vfs: &Vfs<F>,
     folder_entry: &VfsEntry,
     init_name: &str,
 ) -> SnapshotInstanceResult<'static> {

src/snapshot_middleware/middleware.rs

@@ -15,7 +15,7 @@ pub type SnapshotFileResult = Option<(String, VfsSnapshot)>;
 pub trait SnapshotMiddleware {
     fn from_vfs<F: VfsFetcher>(
         context: &mut InstanceSnapshotContext,
-        vfs: &mut Vfs<F>,
+        vfs: &Vfs<F>,
         entry: &VfsEntry,
     ) -> SnapshotInstanceResult<'static>;
 

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<F: VfsFetcher>(
             context: &mut InstanceSnapshotContext,
-            vfs: &mut Vfs<F>,
+            vfs: &Vfs<F>,
             entry: &VfsEntry,
         ) -> SnapshotInstanceResult<'static> {
             $(

src/snapshot_middleware/project.rs

@@ -23,7 +23,7 @@ pub struct SnapshotProject;
 impl SnapshotMiddleware for SnapshotProject {
     fn from_vfs<F: VfsFetcher>(
         context: &mut InstanceSnapshotContext,
-        vfs: &mut Vfs<F>,
+        vfs: &Vfs<F>,
         entry: &VfsEntry,
     ) -> SnapshotInstanceResult<'static> {
         if entry.is_directory() {
@@ -80,7 +80,7 @@ fn snapshot_project_node<F: VfsFetcher>(
     context: &mut InstanceSnapshotContext,
     instance_name: &str,
     node: &ProjectNode,
-    vfs: &mut Vfs<F>,
+    vfs: &Vfs<F>,
 ) -> SnapshotInstanceResult<'static> {
     let name = Cow::Owned(instance_name.to_owned());
     let mut class_name = node

src/snapshot_middleware/rbxlx.rs

@@ -16,7 +16,7 @@ pub struct SnapshotRbxlx;
 impl SnapshotMiddleware for SnapshotRbxlx {
     fn from_vfs<F: VfsFetcher>(
         _context: &mut InstanceSnapshotContext,
-        vfs: &mut Vfs<F>,
+        vfs: &Vfs<F>,
         entry: &VfsEntry,
     ) -> SnapshotInstanceResult<'static> {
         if entry.is_directory() {

src/snapshot_middleware/rbxm.rs

@@ -18,7 +18,7 @@ pub struct SnapshotRbxm;
 impl SnapshotMiddleware for SnapshotRbxm {
     fn from_vfs<F: VfsFetcher>(
         _context: &mut InstanceSnapshotContext,
-        vfs: &mut Vfs<F>,
+        vfs: &Vfs<F>,
         entry: &VfsEntry,
     ) -> SnapshotInstanceResult<'static> {
         if entry.is_directory() {

src/snapshot_middleware/rbxmx.rs

@@ -16,7 +16,7 @@ pub struct SnapshotRbxmx;
 impl SnapshotMiddleware for SnapshotRbxmx {
     fn from_vfs<F: VfsFetcher>(
         _context: &mut InstanceSnapshotContext,
-        vfs: &mut Vfs<F>,
+        vfs: &Vfs<F>,
         entry: &VfsEntry,
     ) -> SnapshotInstanceResult<'static> {
         if entry.is_directory() {

src/snapshot_middleware/txt.rs

@@ -21,7 +21,7 @@ pub struct SnapshotTxt;
 impl SnapshotMiddleware for SnapshotTxt {
     fn from_vfs<F: VfsFetcher>(
         _context: &mut InstanceSnapshotContext,
-        vfs: &mut Vfs<F>,
+        vfs: &Vfs<F>,
         entry: &VfsEntry,
     ) -> SnapshotInstanceResult<'static> {
         if entry.is_directory() {

src/snapshot_middleware/user_plugins.rs

@@ -16,7 +16,7 @@ pub struct SnapshotUserPlugins;
 impl SnapshotMiddleware for SnapshotUserPlugins {
     fn from_vfs<F: VfsFetcher>(
         context: &mut InstanceSnapshotContext,
-        _vfs: &mut Vfs<F>,
+        _vfs: &Vfs<F>,
         _entry: &VfsEntry,
     ) -> SnapshotInstanceResult<'static> {
         // User plugins are only enabled if present on the snapshot context.

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<FileType>;
-    fn read_children(&mut self, path: &Path) -> io::Result<Vec<PathBuf>>;
-    fn read_contents(&mut self, path: &Path) -> io::Result<Vec<u8>>;
+    fn file_type(&self, path: &Path) -> io::Result<FileType>;
+    fn read_children(&self, path: &Path) -> io::Result<Vec<PathBuf>>;
+    fn read_contents(&self, path: &Path) -> io::Result<Vec<u8>>;
 
-    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<VfsEvent>;
 
+    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<PathBuf> {
         Vec::new()
     }
 }

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<FileType> {
+    fn file_type(&self, _path: &Path) -> io::Result<FileType> {
         Err(io::Error::new(
             io::ErrorKind::NotFound,
             "NoopFetcher always returns NotFound",
         ))
     }
 
-    fn read_children(&mut self, _path: &Path) -> io::Result<Vec<PathBuf>> {
+    fn read_children(&self, _path: &Path) -> io::Result<Vec<PathBuf>> {
         Err(io::Error::new(
             io::ErrorKind::NotFound,
             "NoopFetcher always returns NotFound",
         ))
     }
 
-    fn read_contents(&mut self, _path: &Path) -> io::Result<Vec<u8>> {
+    fn read_contents(&self, _path: &Path) -> io::Result<Vec<u8>> {
         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<VfsEvent> {
         crossbeam_channel::never()

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<RecommendedWatcher>,
+    watcher: Option<Mutex<RecommendedWatcher>>,
 
     /// 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<PathBuf>,
+    watched_paths: Mutex<HashSet<PathBuf>>,
 }
 
 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<DebouncedEvent>, sender: Sen
 }
 
 impl VfsFetcher for RealFetcher {
-    fn file_type(&mut self, path: &Path) -> io::Result<FileType> {
+    fn file_type(&self, path: &Path) -> io::Result<FileType> {
         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<Vec<PathBuf>> {
+    fn read_children(&self, path: &Path) -> io::Result<Vec<PathBuf>> {
         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<Vec<u8>> {
+    fn read_contents(&self, path: &Path) -> io::Result<Vec<u8>> {
         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<PathBuf> {
+        let watched_paths = self.watched_paths.lock().unwrap();
+        watched_paths.iter().map(|v| v.clone()).collect()
     }
 }

src/vfs/test_fetcher.rs

@@ -107,7 +107,7 @@ impl TestFetcher {
 }
 
 impl VfsFetcher for TestFetcher {
-    fn file_type(&mut self, path: &Path) -> io::Result<FileType> {
+    fn file_type(&self, path: &Path) -> io::Result<FileType> {
         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<Vec<PathBuf>> {
+    fn read_children(&self, path: &Path) -> io::Result<Vec<PathBuf>> {
         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<Vec<u8>> {
+    fn read_contents(&self, path: &Path) -> io::Result<Vec<u8>> {
         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<VfsEvent> {
         self.receiver.clone()
     }

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<F> {
     /// A hierarchical map from paths to items that have been read or partially
     /// read into memory by the Vfs.
-    data: PathMap<VfsItem>,
+    data: Mutex<PathMap<VfsItem>>,
 
     /// 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<F> {
 impl<F: VfsFetcher> Vfs<F> {
     pub fn new(fetcher: F) -> Self {
         Self {
-            data: PathMap::new(),
+            data: Mutex::new(PathMap::new()),
             fetcher,
         }
     }
@@ -48,108 +48,37 @@ impl<F: VfsFetcher> Vfs<F> {
         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<Path>) -> 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<Path>) -> FsResult<VfsEntry> {
+        let mut data = self.data.lock().unwrap();
+        Self::get_internal(&mut data, &self.fetcher, path)
     }
 
-    fn raise_file_removed(&mut self, path: impl AsRef<Path>) -> FsResult<()> {
+    pub fn get_contents(&self, path: impl AsRef<Path>) -> FsResult<Arc<Vec<u8>>> {
         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<Path>) -> FsResult<VfsEntry> {
-        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<Path>) -> FsResult<Arc<Vec<u8>>> {
-        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<F: VfsFetcher> Vfs<F> {
         }
     }
 
-    pub fn get_children(&mut self, path: impl AsRef<Path>) -> FsResult<Vec<VfsEntry>> {
+    pub fn get_children(&self, path: impl AsRef<Path>) -> FsResult<Vec<VfsEntry>> {
         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::<Vec<PathBuf>>() // 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::<FsResult<Vec<VfsEntry>>>()
                 } else {
                     dir.children_enumerated = true;
@@ -197,7 +126,7 @@ impl<F: VfsFetcher> Vfs<F> {
                         .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::<FsResult<Vec<VfsEntry>>>()
                 }
             }
@@ -208,6 +137,118 @@ impl<F: VfsFetcher> Vfs<F> {
         }
     }
 
+    fn get_internal(
+        data: &mut PathMap<VfsItem>,
+        fetcher: &F,
+        path: impl AsRef<Path>,
+    ) -> FsResult<VfsEntry> {
+        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<VfsItem>,
+        fetcher: &F,
+        path: impl AsRef<Path>,
+    ) -> 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<VfsItem>,
+        fetcher: &F,
+        path: impl AsRef<Path>,
+    ) -> 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<VfsItem>, 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<F: VfsFetcher> Vfs<F> {
     /// 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<VfsItem>, 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<P: AsRef<Path>>(&mut self, path: P, snapshot: VfsSnapshot);
+    fn debug_load_snapshot<P: AsRef<Path>>(&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<Arc<Vec<u8>>>;
+    fn debug_children(&self, path: &Path) -> Option<(bool, Vec<PathBuf>)>;
+    fn debug_orphans(&self) -> Vec<PathBuf>;
+    fn debug_watched_paths(&self) -> Vec<PathBuf>;
 }
 
 impl<F: VfsFetcher> VfsDebug for Vfs<F> {
-    fn debug_load_snapshot<P: AsRef<Path>>(&mut self, path: P, snapshot: VfsSnapshot) {
-        let path = path.as_ref();
+    fn debug_load_snapshot<P: AsRef<Path>>(&self, path: P, snapshot: VfsSnapshot) {
+        fn load_snapshot<P: AsRef<Path>>(
+            data: &mut PathMap<VfsItem>,
+            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<Arc<Vec<u8>>> {
+        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<PathBuf>)> {
+        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<PathBuf> {
+        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<PathBuf> {
         self.fetcher.watched_paths()
     }
 }
@@ -345,11 +381,11 @@ impl VfsEntry {
         &self.path
     }
 
-    pub fn contents<'vfs>(&self, vfs: &'vfs mut Vfs<impl VfsFetcher>) -> FsResult<Arc<Vec<u8>>> {
+    pub fn contents(&self, vfs: &Vfs<impl VfsFetcher>) -> FsResult<Arc<Vec<u8>>> {
         vfs.get_contents(&self.path)
     }
 
-    pub fn children(&self, vfs: &mut Vfs<impl VfsFetcher>) -> FsResult<Vec<VfsEntry>> {
+    pub fn children(&self, vfs: &Vfs<impl VfsFetcher>) -> FsResult<Vec<VfsEntry>> {
         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<FileType> {
+            fn file_type(&self, path: &Path) -> io::Result<FileType> {
                 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<Vec<u8>> {
+            fn read_contents(&self, path: &Path) -> io::Result<Vec<u8>> {
                 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<Vec<PathBuf>> {
+            fn read_children(&self, _path: &Path) -> io::Result<Vec<PathBuf>> {
                 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<VfsEvent> {
                 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") {

src/web/ui.rs

@@ -102,7 +102,7 @@ impl<F: VfsFetcher> UiService<F> {
         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<F: VfsFetcher> UiService<F> {
 
             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 {