DioxusLabs / dioxus

Fullstack app framework for web, desktop, mobile, and more.
https://dioxuslabs.com
Apache License 2.0
20.46k stars 786 forks source link

Panicking in `cx.spawn` task should not bring down the entire app #632

Open jkelleyrtp opened 1 year ago

jkelleyrtp commented 1 year ago

Specific Demand

In react, if you throw an exception, it'll get caught by the react machinery as an error at the nearest error boundary.

WASM does not have the ability to catch panics properly, but non-wasm targets do.

We should catch panics in tasks and suspense and bubble them to the nearest error boundary if possible.

This is pretty straightforward by simply wrapping the scheduler poll logic with a panic handler.

Implement Suggestion

This comes in two parts

jkelleyrtp commented 2 months ago

From evan in the discord:

Have you run into unwind safety before? Both tasks and event handling use interior mutability and can run user code which contains interior mutability. That causes trying to catch unwinds to give an error about unwind safety We can assert that it is unwind safe, but I'm not sure it is This isn't a safety issue, but it can cause logic errors

Types such as &mut T and &RefCell are examples which are not unwind safe. The general idea is that any mutable state which can be shared across catch_unwind is not unwind safe by default. This is because it is very easy to witness a broken invariant outside of catch_unwind as the data is simply accessed as usual.


Seems like it could be possible, but requires us to think about the invariants we're upholding in specific chunks of the codebase.

Do we wrap .poll() or do we wrap the whole method? Where do we need to focus on maintaining invariants?

Ideally this is just a debug_assertions feature so invariants aren't terribly important - double panics are okay in debug mode.

chungwong commented 1 month ago

Should this issue be considered adding to the 0.6 release/milestone? Given that panics could happens in other deps and potentially uncontrolled, it is very easy to crash the entire app.

ealmloff commented 1 month ago

Should this issue be considered adding to the 0.6 release/milestone? Given that panics could happens in other deps and potentially uncontrolled, it is very easy to crash the entire app.

Libraries should return Results for actions that can fail. This should help avoid issues with unexpected panics on desktop, but it shouldn't be something you rely on for known panics. This isn't possible to implement consistently across all platforms because rust compiled to wasm doesn't support unwinding. It should act more as a safe guard if all other error handling fails

chungwong commented 1 month ago

Maybe I should talk about the issue with concrete examples rather than hypothetical. There are lots of cases where a function can panics.

  1. Vec::with_capacity
  2. Vec::windows

Of course, those panics are known and arguably can be handled better. Let's actually get to the conrete part and focus on https://dioxuslabs.com which is an excellent example to demostrate the concern I have.

https://github.com/DioxusLabs/docsite/issues/288 https://github.com/DioxusLabs/docsite/issues/218 https://discord.com/channels/899851952891002890/1275886621597634670

Because of the panics, none of the WASM stuff worked, coutner example didn't work, search didn't work, the nav hamburger didn't work. So panics are actually unpredictable and the results are not desirable in practice.