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

Panic in model crashes the app #1519

Closed
hannobraun opened this issue Jan 18, 2023 · 2 comments · Fixed by #1534
Closed

Panic in model crashes the app #1519

hannobraun opened this issue Jan 18, 2023 · 2 comments · Fixed by #1534
Labels
good first issue Good for newcomers type: bug Something isn't working

Comments

@hannobraun
Copy link
Owner

Currently, if model code panics, this crashes the whole app:

thread '<unnamed>' panicked at 'not yet implemented', models/test/src/lib.rs:7:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
not yet implemented
Aborted (core dumped)

This can be easily reproduced by adding a todo!()/panic!()/... to any model.

Panics can be a normal part of creating a model. They can happen accidentally, or even on purpose, when placing a todo!() somewhere. The app should handle that case well, and just show an error message under "Status".

fj-host, which is responsible for handling models, already expects that any interaction with a model can result in an error, and reports those errors to the GUI. I believe that no changes are required there. So we just need to make sure that whenever model code panics, this panic is converted into an error and returned to fj-host.

(It would be advantageous to instead catch panics on the host's side, to protect against malicious models. But I consider such protection a nice-to-have at this point, and out of scope of this issue. In addition, we need to catch those panics on the model side, as panics over an FFI boundary are undefined behavior, as far as I know. Maybe we can do better once we switch to WebAssembly.)

It looks like panics in the model are already handled, so all we need to do is check where that handler function is called, determine if it's practical to return an error there instead, and adapt the code to do so.

Labeling https://github.com/hannobraun/Fornjot/labels/good%20first%20issue, as the scope of this problem is limited to a specific part of Fornjot, and knowledge about the rest of the system shouldn't be required to address this.

@hannobraun hannobraun added type: bug Something isn't working good first issue Good for newcomers topic: host labels Jan 18, 2023
@mxdamien
Copy link
Contributor

Hey and happy new year ! :)

See #1534 for a suggestion

  • David

@hannobraun
Copy link
Owner Author

Hey @mxdamien, nice to have you back! I'll take a look at that pull request now.

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

Successfully merging a pull request may close this issue.

2 participants