Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Model modifications happening during fj-host initialization could be missed #32

Closed
hannobraun opened this issue Jan 9, 2022 · 1 comment · Fixed by #380
Closed

Model modifications happening during fj-host initialization could be missed #32

hannobraun opened this issue Jan 9, 2022 · 1 comment · Fixed by #380
Labels
type: bug Something isn't working

Comments

@hannobraun
Copy link
Owner

The initialization of the host application roughly looks like this:

  1. Load the model.
  2. Do a bunch of other stuff.
  3. Set up the mechanism that watches the model for changes.

If the model were to be modified during step 2., that change would be missed. I don't think this is very likely, and have certainly never seen it in real use, but none the less, there's a race condition here.

I haven't looked into this in a while, but last time I did, I got the impression that it isn't easy to fix, and probably a rework of that aspect of the architecture would be required.

In case anyone wants to take a look, here's where the model is loaded:

fornjot/src/main.rs

Lines 53 to 59 in 416df5f

// 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.
let shape = model.load(&parameters)?;

And here's where the watcher is set up:

fornjot/src/main.rs

Lines 123 to 163 in 416df5f

let watch_path = model.src_path();
let mut watcher = notify::recommended_watcher(
move |event: notify::Result<notify::Event>| {
// TASK: Figure out when this error can happen, find a better way to
// handle it.
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 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)?;

@hannobraun hannobraun added type: bug Something isn't working topic: model labels Jan 9, 2022
@hannobraun
Copy link
Owner Author

I don't plan to work on this, as I don't think this is likely to happen in practice. And since more changes to the model/plugin functionality are going to happen (see #7), I hope that a cleaner architecture that takes care of this problem will reveal itself during that process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant