Merge pull request #380 from hannobraun/race

Fix race condition when loading model initially

fj-app/src/main.rs

@@ -7,16 +7,13 @@ mod mesh;
 mod model;
 mod window;
 
-use std::collections::HashSet;
-use std::ffi::OsStr;
 use std::path::PathBuf;
-use std::{collections::HashMap, sync::mpsc, time::Instant};
+use std::{collections::HashMap, time::Instant};
 
 use fj_debug::DebugInfo;
 use fj_math::{Aabb, Scalar, Triangle};
 use fj_operations::ToShape as _;
 use futures::executor::block_on;
-use notify::Watcher as _;
 use tracing::trace;
 use tracing_subscriber::fmt::format;
 use tracing_subscriber::EnvFilter;
@@ -83,24 +80,15 @@ fn main() -> anyhow::Result<()> {
         parameters.insert(key, value);
     }
 
-    // Since we're loading the model before setting up the watcher below,
-    // there's a race condition, and a modification could be missed between
-    // those two events.
-    //
-    // This can't be addressed with the current structure, since the watcher
-    // closure takes ownership of the model.
-    //
-    // This is being tracked in the following issue:
-    // https://github.com/hannobraun/fornjot/issues/32
-    let shape = model.load(&parameters)?;
-
     let shape_processor = ShapeProcessor::new(args.tolerance)?;
-    let mut processed_shape = shape_processor.process(&shape);
 
     if let Some(path) = args.export {
+        let shape = model.load_once(&parameters)?;
+        let shape = shape_processor.process(&shape);
+
         let mut mesh_maker = MeshMaker::new();
 
-        for triangle in processed_shape.triangles {
+        for triangle in shape.triangles {
             for vertex in triangle.points() {
                 mesh_maker.push(vertex);
             }
@@ -131,67 +119,7 @@ fn main() -> anyhow::Result<()> {
         return Ok(());
     }
 
-    let (watcher_tx, watcher_rx) = mpsc::sync_channel(0);
+    let watcher = model.load_and_watch(parameters)?;
-
-    let watch_path = model.src_path();
-    let mut watcher = notify::recommended_watcher(
-        move |event: notify::Result<notify::Event>| {
-            // Unfortunately the `notify` documentation doesn't say when this
-            // might happen, so no idea if it needs to be handled.
-            let event = event.expect("Error handling watch event");
-
-            //Various acceptable ModifyKind kinds. Varies across platforms (e.g. MacOs vs. Windows10)
-            if let notify::EventKind::Modify(notify::event::ModifyKind::Any)
-            | notify::EventKind::Modify(notify::event::ModifyKind::Data(
-                notify::event::DataChange::Any,
-            ))
-            | notify::EventKind::Modify(notify::event::ModifyKind::Data(
-                notify::event::DataChange::Content,
-            )) = event.kind
-            {
-                let file_ext = event
-                    .paths
-                    .get(0)
-                    .expect("File path missing in watch event")
-                    .extension();
-
-                let black_list = HashSet::from([
-                    OsStr::new("swp"),
-                    OsStr::new("tmp"),
-                    OsStr::new("swx"),
-                ]);
-
-                if let Some(ext) = file_ext {
-                    if black_list.contains(ext) {
-                        return;
-                    }
-                }
-
-                let shape = match model.load(&parameters) {
-                    Ok(shape) => shape,
-                    Err(model::Error::Compile) => {
-                        // It would be better to display an error in the UI,
-                        // where the user can actually see it. Issue:
-                        // https://github.com/hannobraun/fornjot/issues/30
-                        println!("Error compiling model");
-                        return;
-                    }
-                    Err(err) => {
-                        panic!("Error reloading model: {:?}", err);
-                    }
-                };
-
-                // This will panic, if the other end is disconnected, which is
-                // probably the result of a panic on that thread, or the
-                // application is being shut down.
-                //
-                // Either way, not much we can do about it here, except maybe to
-                // provide a better error message in the future.
-                watcher_tx.send(shape).unwrap();
-            }
-        },
-    )?;
-    watcher.watch(&watch_path, notify::RecursiveMode::Recursive)?;
 
     let event_loop = EventLoop::new();
     let window = Window::new(&event_loop);
@@ -201,10 +129,10 @@ fn main() -> anyhow::Result<()> {
     let mut input_handler = input::Handler::new(previous_time);
     let mut renderer = block_on(Renderer::new(&window))?;
 
-    processed_shape.update_geometry(&mut renderer);
-
     let mut draw_config = DrawConfig::default();
-    let mut camera = Camera::new(&processed_shape.aabb);
+
+    let mut shape = None;
+    let mut camera = None;
 
     event_loop.run(move |event, _, control_flow| {
         trace!("Handling event: {:?}", event);
@@ -213,20 +141,15 @@ fn main() -> anyhow::Result<()> {
 
         let now = Instant::now();
 
-        match watcher_rx.try_recv() {
+        if let Some(new_shape) = watcher.receive() {
-            Ok(shape) => {
+            let new_shape = shape_processor.process(&new_shape);
-                processed_shape = shape_processor.process(&shape);
+            new_shape.update_geometry(&mut renderer);
-                processed_shape.update_geometry(&mut renderer);
+
-            }
+            if camera.is_none() {
-            Err(mpsc::TryRecvError::Empty) => {
+                camera = Some(Camera::new(&new_shape.aabb));
-                // Nothing to receive from the channel. We don't care.
-            }
-            Err(mpsc::TryRecvError::Disconnected) => {
-                // The other end has disconnected. This is probably the result
-                // of a panic on the other thread, or a program shutdown in
-                // progress. In any case, not much we can do here.
-                panic!();
             }
+
+            shape = Some(new_shape);
         }
 
         match event {
@@ -252,23 +175,28 @@ fn main() -> anyhow::Result<()> {
                 event: WindowEvent::CursorMoved { position, .. },
                 ..
             } => {
-                input_handler.handle_cursor_moved(
+                if let Some(camera) = &mut camera {
-                    position,
+                    input_handler
-                    &mut camera,
+                        .handle_cursor_moved(position, camera, &window);
-                    &window,
+                }
-                );
             }
             Event::WindowEvent {
                 event: WindowEvent::MouseInput { state, button, .. },
                 ..
             } => {
-                let focus_point = camera.focus_point(
+                if let (Some(shape), Some(camera)) = (&shape, &camera) {
-                    &window,
+                    let focus_point = camera.focus_point(
-                    input_handler.cursor(),
+                        &window,
-                    &processed_shape.triangles,
+                        input_handler.cursor(),
-                );
+                        &shape.triangles,
+                    );
 
-                input_handler.handle_mouse_input(button, state, focus_point);
+                    input_handler.handle_mouse_input(
+                        button,
+                        state,
+                        focus_point,
+                    );
+                }
             }
             Event::WindowEvent {
                 event: WindowEvent::MouseWheel { delta, .. },
@@ -280,23 +208,27 @@ fn main() -> anyhow::Result<()> {
                 let delta_t = now.duration_since(previous_time);
                 previous_time = now;
 
-                input_handler.update(
+                if let (Some(shape), Some(camera)) = (&shape, &mut camera) {
-                    delta_t.as_secs_f64(),
+                    input_handler.update(
-                    now,
+                        delta_t.as_secs_f64(),
-                    &mut camera,
+                        now,
-                    &window,
+                        camera,
-                    &processed_shape.triangles,
+                        &window,
-                );
+                        &shape.triangles,
+                    );
+                }
 
                 window.inner().request_redraw();
             }
             Event::RedrawRequested(_) => {
-                camera.update_planes(&processed_shape.aabb);
+                if let (Some(shape), Some(camera)) = (&shape, &mut camera) {
+                    camera.update_planes(&shape.aabb);
 
-                match renderer.draw(&camera, &draw_config) {
+                    match renderer.draw(camera, &draw_config) {
-                    Ok(()) => {}
+                        Ok(()) => {}
-                    Err(err) => {
+                        Err(err) => {
-                        panic!("Draw error: {}", err);
+                            panic!("Draw error: {}", err);
+                        }
                     }
                 }
             }

fj-app/src/model.rs

@@ -1,5 +1,14 @@
-use std::{collections::HashMap, io, path::PathBuf, process::Command};
+use std::{
+    collections::{HashMap, HashSet},
+    ffi::OsStr,
+    io,
+    path::PathBuf,
+    process::Command,
+    sync::mpsc,
+    thread,
+};
 
+use notify::Watcher as _;
 use thiserror::Error;
 
 pub struct Model {
@@ -47,11 +56,7 @@ impl Model {
         })
     }
 
-    pub fn src_path(&self) -> PathBuf {
+    pub fn load_once(
-        self.src_path.clone()
-    }
-
-    pub fn load(
         &self,
         arguments: &HashMap<String, String>,
     ) -> Result<fj::Shape, Error> {
@@ -90,6 +95,125 @@ impl Model {
 
         Ok(shape)
     }
+
+    pub fn load_and_watch(
+        self,
+        parameters: HashMap<String, String>,
+    ) -> Result<Watcher, Error> {
+        let (tx, rx) = mpsc::sync_channel(0);
+        let tx2 = tx.clone();
+
+        let watch_path = self.src_path.clone();
+
+        let mut watcher = notify::recommended_watcher(
+            move |event: notify::Result<notify::Event>| {
+                // Unfortunately the `notify` documentation doesn't say when
+                // this might happen, so no idea if it needs to be handled.
+                let event = event.expect("Error handling watch event");
+
+                // Various acceptable ModifyKind kinds. Varies across platforms
+                // (e.g. MacOs vs. Windows10)
+                if let notify::EventKind::Modify(
+                    notify::event::ModifyKind::Any,
+                )
+                | notify::EventKind::Modify(
+                    notify::event::ModifyKind::Data(
+                        notify::event::DataChange::Any,
+                    ),
+                )
+                | notify::EventKind::Modify(
+                    notify::event::ModifyKind::Data(
+                        notify::event::DataChange::Content,
+                    ),
+                ) = event.kind
+                {
+                    let file_ext = event
+                        .paths
+                        .get(0)
+                        .expect("File path missing in watch event")
+                        .extension();
+
+                    let black_list = HashSet::from([
+                        OsStr::new("swp"),
+                        OsStr::new("tmp"),
+                        OsStr::new("swx"),
+                    ]);
+
+                    if let Some(ext) = file_ext {
+                        if black_list.contains(ext) {
+                            return;
+                        }
+                    }
+
+                    // This will panic, if the other end is disconnected, which
+                    // is probably the result of a panic on that thread, or the
+                    // application is being shut down.
+                    //
+                    // Either way, not much we can do about it here, except
+                    // maybe to provide a better error message in the future.
+                    tx.send(()).unwrap();
+                }
+            },
+        )?;
+
+        watcher.watch(&watch_path, notify::RecursiveMode::Recursive)?;
+
+        // To prevent a race condition between the initial load and the start of
+        // watching, we'll trigger the initial load here, after having started
+        // watching.
+        //
+        // Will panic, if the receiving end has panicked. Not much we can do
+        // about that, if it happened.
+        thread::spawn(move || tx2.send(()).unwrap());
+
+        Ok(Watcher {
+            _watcher: Box::new(watcher),
+            channel: rx,
+            model: self,
+            parameters,
+        })
+    }
+}
+
+pub struct Watcher {
+    _watcher: Box<dyn notify::Watcher>,
+    channel: mpsc::Receiver<()>,
+    model: Model,
+    parameters: HashMap<String, String>,
+}
+
+impl Watcher {
+    pub fn receive(&self) -> Option<fj::Shape> {
+        match self.channel.try_recv() {
+            Ok(()) => {
+                let shape = match self.model.load_once(&self.parameters) {
+                    Ok(shape) => shape,
+                    Err(Error::Compile) => {
+                        // It would be better to display an error in the UI,
+                        // where the user can actually see it. Issue:
+                        // https://github.com/hannobraun/fornjot/issues/30
+                        println!("Error compiling model");
+                        return None;
+                    }
+                    Err(err) => {
+                        panic!("Error reloading model: {:?}", err);
+                    }
+                };
+
+                Some(shape)
+            }
+            Err(mpsc::TryRecvError::Empty) => {
+                // Nothing to receive from the channel.
+                None
+            }
+            Err(mpsc::TryRecvError::Disconnected) => {
+                // The other end has disconnected. This is probably the result
+                // of a panic on the other thread, or a program shutdown in
+                // progress. In any case, not much we can do here.
+                panic!();
+            }
+        }
+    }
 }
 
 #[derive(Debug, Error)]
@@ -102,6 +226,9 @@ pub enum Error {
 
     #[error("Error loading model from dynamic library")]
     LibLoading(#[from] libloading::Error),
+
+    #[error("Error watching model for changes")]
+    Notify(#[from] notify::Error),
 }
 
 type ModelFn =