Discussions about UnwindSafe/RefUnwindSafe

[fzyzcjy]

Background

Firstly, it is inevitable to use catch_unwind, because we surely do not want the whole app to crash (Rust's "abort") if there is just a Rust panic. Instead, we want an exception thrown in Dart side. That's the current FRB behavior.

Secondly, when using catch_unwind, Rust requires the related things are UnwindSafe. When something is not automatically considered as UnwindSafe by the Rust compiler, the programmer needs to do something manually. One way is to use AssertUnwindSafe, which is safe Rust code (not unsafe Rust code! so no big worries).

The question

Approach 1: Currently, I choose to leave it to the end user, i.e. the users need to manually think and wrap AssertUnwindSafe here and there.

Approach 2: An alternative approach is to wrap AssertUnwindSafe unconditionally inside FRB core, thus the users do not need to worry about this, which makes development a bit more fluent, and also avoid some edge cases like #1573.

Related: Discussions around AssertUnwindSafe

e.g. https://users.rust-lang.org/t/is-this-use-of-assertunwindsafe-safe-wrapping-a-future-that-is-send-static/73067

Is it safe? Well, you have no unsafe block, so the code doesn't have UB.
That said, your code can silence the kind of issue that unwind safety tries to protect you from. I don't know whether that matters to you - most people don't care about unwind safety.

As for what to do if a type is not UnwindSafe:

Well, you could wrap each of those values in an AssertUnwindSafe, or your own custom struct that you explicitly impl the UnwindSafe trait for.
You can also submit issues/PRs to those crates to mark the types as unwind safe.

Given alice's thoughts, it seems that it is good to use approach 2.

rust-lang/rust#40628

I've seen now quite a bit of people that told me that "they just wrap everything in AssertUnwindSafe" defeating the whole purpose of it.

UnwindSafe never protects from UB, it's there to protect against logic errors.

Following hyperlinks here and there, I see notes such as:

rust-lang/crates.io#7453

rust-lang/rust#40628, rust-lang/rust#65717 and rust-lang/rfcs#3260 all show that unwind safety isn't particularly ergonomic to use and implement, and ultimately leads to people slapping AssertUnwindSafe everywhere until the compiler stops complaining.
This situation has led to built-in test framework using catch_unwind(AssertUnwindSafe(...)) (see rust-lang/[email protected]/library/test/src/lib.rs#L649) and libraries like tower-http doing the same (see docs.rs/tower-http/0.4.4/src/tower_http/catch_panic.rs.html#198).
As people have mentioned in the threads above, trying to implement this correctly is akin to fighting windmills at the moment. Since the above cases demonstrated that catch_unwind(AssertUnwindSafe(...)) is currently the easiest way to deal with this situation, this commit does the same and refactors our background job runner code accordingly.

I quickly searched Pyo3:
https://github.com/search?q=repo%3APyO3%2Fpyo3+AssertUnwindSafe&type=code
And during my usage of pyo3, I am never required to add AssertUnwindSafe here and there.
Therefore, without deeper investigation, I guess pyo3 uses approach 2.

[fzyzcjy]

cc @Desdaemon - what do you think?

(Everyone feel free to discuss here! But I guess this issue will be seen by almost nobody except when @ someone, since IMHO few people will go through the issue tracker and open each issue to have a look.)

[JustSimplyKyle]

Personally I think approach two is alright, but maybe add an opt-out?(so that in the rare case if the user does care about unwindsafety they can opt-out

[fzyzcjy]

Totally agree. Maybe we can firstly use approach 2, and when someone in the future wants to opt-out, he or she can create an issue, and we can implement that in the future.

[fzyzcjy]

Tentative: #1574

[fzyzcjy]

One more related user report: #1575

[Desdaemon]

Personally, the largest source of friction for me has been wrapping trait objects since its implementers are not guaranteed to be unwind-safe despite being so most of the time. Otherwise I appreciate the fine-grained approach to safety and don't mind a bit boilerplate, but it's good to have options.

[fzyzcjy]

Looks reasonable! Then Maybe I will firstly use approach 2 to fix those several bug reports immediately, and consider approach 1 later.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new issue.