Improve IMFS robustness with out-of-order events

Fixes #111.

.gitignore

@@ -1,5 +1,5 @@
 /site
 /target
-/server/scratch
+/scratch-project
 **/*.rs.bk
 /generate-docs.run

CHANGELOG.md

@@ -1,6 +1,7 @@
 # Rojo Changelog
 
 ## [Unreleased]
+* Fixed in-memory filesystem not handling out-of-order filesystem change events
 
 ## [0.5.0 Alpha 1](https://github.com/LPGhatguy/rojo/releases/tag/v0.5.0-alpha.1) (January 14, 2019)
 * Changed plugin UI to be way prettier

server/src/fs_watcher.rs

@@ -4,7 +4,7 @@ use std::{
     thread,
 };
 
-use log::info;
+use log::trace;
 use notify::{
     self,
     DebouncedEvent,
@@ -23,6 +23,8 @@ const WATCH_TIMEOUT: Duration = Duration::from_millis(100);
 fn handle_event(imfs: &Mutex<Imfs>, rbx_session: &Mutex<RbxSession>, event: DebouncedEvent) {
     match event {
         DebouncedEvent::Create(path) => {
+            trace!("Path created: {}", path.display());
+
             {
                 let mut imfs = imfs.lock().unwrap();
                 imfs.path_created(&path).unwrap();
@@ -34,6 +36,8 @@ fn handle_event(imfs: &Mutex<Imfs>, rbx_session: &Mutex<RbxSession>, event: Debo
             }
         },
         DebouncedEvent::Write(path) => {
+            trace!("Path created: {}", path.display());
+
             {
                 let mut imfs = imfs.lock().unwrap();
                 imfs.path_updated(&path).unwrap();
@@ -45,6 +49,8 @@ fn handle_event(imfs: &Mutex<Imfs>, rbx_session: &Mutex<RbxSession>, event: Debo
             }
         },
         DebouncedEvent::Remove(path) => {
+            trace!("Path removed: {}", path.display());
+
             {
                 let mut imfs = imfs.lock().unwrap();
                 imfs.path_removed(&path).unwrap();
@@ -56,6 +62,8 @@ fn handle_event(imfs: &Mutex<Imfs>, rbx_session: &Mutex<RbxSession>, event: Debo
             }
         },
         DebouncedEvent::Rename(from_path, to_path) => {
+            trace!("Path rename: {} to {}", from_path.display(), to_path.display());
+
             {
                 let mut imfs = imfs.lock().unwrap();
                 imfs.path_moved(&from_path, &to_path).unwrap();
@@ -66,7 +74,9 @@ fn handle_event(imfs: &Mutex<Imfs>, rbx_session: &Mutex<RbxSession>, event: Debo
                 rbx_session.path_renamed(&from_path, &to_path);
             }
         },
-        _ => {},
+        other => {
+            trace!("Unhandled FS event: {:?}", other);
+        },
     }
 }
 
@@ -100,11 +110,11 @@ impl FsWatcher {
                 let root_path = root_path.clone();
 
                 thread::spawn(move || {
-                    info!("Watcher thread ({}) started", root_path.display());
+                    trace!("Watcher thread ({}) started", root_path.display());
                     while let Ok(event) = watch_rx.recv() {
                         handle_event(&imfs, &rbx_session, event);
                     }
-                    info!("Watcher thread ({}) stopped", root_path.display());
+                    trace!("Watcher thread ({}) stopped", root_path.display());
                 });
             }
         }

server/src/imfs.rs

@@ -1,6 +1,6 @@
 use std::{
     collections::{HashMap, HashSet},
-    path::{Path, PathBuf},
+    path::{self, Path, PathBuf},
     fs,
     io,
 };
@@ -24,9 +24,12 @@ fn add_sync_points(imfs: &mut Imfs, project_node: &ProjectNode) -> io::Result<()
     Ok(())
 }
 
-/// The in-memory filesystem keeps a mirror of all files being watcher by Rojo
+/// The in-memory filesystem keeps a mirror of all files being watched by Rojo
 /// in order to deduplicate file changes in the case of bidirectional syncing
 /// from Roblox Studio.
+///
+/// It also enables Rojo to quickly generate React-like snapshots to make
+/// reasoning about instances and how they relate to files easier.
 #[derive(Debug, Clone)]
 pub struct Imfs {
     items: HashMap<PathBuf, ImfsItem>,
@@ -66,21 +69,21 @@ impl Imfs {
 
         self.roots.insert(path.to_path_buf());
 
-        self.read_from_disk(path)
+        self.descend_and_read_from_disk(path)
     }
 
     pub fn path_created(&mut self, path: &Path) -> io::Result<()> {
         debug_assert!(path.is_absolute());
         debug_assert!(self.is_within_roots(path));
 
-        self.read_from_disk(path)
+        self.descend_and_read_from_disk(path)
     }
 
     pub fn path_updated(&mut self, path: &Path) -> io::Result<()> {
         debug_assert!(path.is_absolute());
         debug_assert!(self.is_within_roots(path));
 
-        self.read_from_disk(path)
+        self.descend_and_read_from_disk(path)
     }
 
     pub fn path_removed(&mut self, path: &Path) -> io::Result<()> {
@@ -132,9 +135,7 @@ impl Imfs {
             Some(ImfsItem::Directory(directory)) => {
                 directory.children.remove(child);
             },
-            _ => {
+            _ => {},
-                panic!("Tried to unlink child of path that wasn't a directory!");
-            },
         }
     }
 
@@ -153,6 +154,38 @@ impl Imfs {
         }
     }
 
+    fn descend_and_read_from_disk(&mut self, path: &Path) -> io::Result<()> {
+        let root_path = self.get_root_path(path)
+            .expect("Tried to mkdirp for path that wasn't within roots!");
+
+        // If this path is a root, we should read the entire thing.
+        if root_path == path {
+            self.read_from_disk(path)?;
+            return Ok(());
+        }
+
+        let relative_path = path.strip_prefix(root_path).unwrap();
+        let mut current_path = root_path.to_path_buf();
+
+        for component in relative_path.components() {
+            match component {
+                path::Component::Normal(name) => {
+                    let next_path = current_path.join(name);
+
+                    if self.items.contains_key(&next_path) {
+                        current_path = next_path;
+                    } else {
+                        self.read_from_disk(&current_path)?;
+                        break;
+                    }
+                },
+                _ => unreachable!(),
+            }
+        }
+
+        Ok(())
+    }
+
     fn read_from_disk(&mut self, path: &Path) -> io::Result<()> {
         let metadata = fs::metadata(path)?;
 
@@ -195,6 +228,16 @@ impl Imfs {
         }
     }
 
+    fn get_root_path<'a>(&'a self, path: &Path) -> Option<&'a Path> {
+        for root_path in &self.roots {
+            if path.starts_with(root_path) {
+                return Some(root_path)
+            }
+        }
+
+        None
+    }
+
     fn is_within_roots(&self, path: &Path) -> bool {
         for root_path in &self.roots {
             if path.starts_with(root_path) {

server/test-scratch-project

@@ -1,18 +0,0 @@
-#!/bin/sh
-
-set -e
-
-if [ ! -d "../test-projects/$1" ]
-then
-	echo "Pick a project that exists!"
-	exit 1
-fi
-
-if [ -d "scratch" ]
-then
-	rm -rf scratch
-fi
-
-mkdir -p scratch
-cp -r "../test-projects/$1" scratch
-cargo run -- serve "scratch/$1"

server/tests/imfs.rs

@@ -232,6 +232,16 @@ fn adding_folder() -> io::Result<()> {
             FsEvent::Created(file1_path.clone()),
             FsEvent::Created(file2_path.clone()),
         ],
+        vec![
+            FsEvent::Created(file1_path.clone()),
+            FsEvent::Created(file2_path.clone()),
+            FsEvent::Created(folder_path.clone()),
+        ],
+        vec![
+            FsEvent::Created(file1_path.clone()),
+            FsEvent::Created(folder_path.clone()),
+            FsEvent::Created(file2_path.clone()),
+        ],
     ];
 
     for events in &possible_event_sequences {
@@ -294,6 +304,10 @@ fn removing_folder() -> io::Result<()> {
             FsEvent::Removed(resources.baz_path.clone()),
             FsEvent::Removed(resources.foo_path.clone()),
         ],
+        vec![
+            FsEvent::Removed(resources.foo_path.clone()),
+            FsEvent::Removed(resources.baz_path.clone()),
+        ],
     ];
 
     for events in &possible_event_sequences {

test-scratch-project

@@ -0,0 +1,21 @@
+#!/bin/sh
+
+# Copies a project from 'test-projects' into a folder that can be messed with
+# without accidentally checking the results into version control.
+
+set -e
+
+if [ ! -d "test-projects/$1" ]
+then
+	echo "Pick a project that exists!"
+	exit 1
+fi
+
+if [ -d "scratch-project/$1" ]
+then
+	rm -rf "scratch-project/$1"
+fi
+
+mkdir -p scratch-project
+cp -r "test-projects/$1" scratch-project
+cargo run -- serve "scratch-project/$1"