yutannihilation / savvy

A simple R extension interface using Rust
https://yutannihilation.github.io/savvy/guide/
MIT License
63 stars 3 forks source link

panic in other rust's crates #163

Closed daniellga closed 5 months ago

daniellga commented 5 months ago

What can I do to avoid crashing my R session when other dependencies panic? For example I am using ndarray and they have this method that panics.

yutannihilation commented 5 months ago

There's no other way than being careful to satisfy the requirements of the dependency. Savvy might provide some "Debug mode" build, which wraps everything with catch_unwind(), to diagnose what's the problem, but, in the end, the framework doesn't provide any workaround.

In the example case, you can just check the dimension, can't you?

daniellga commented 5 months ago

Sure I thought about that, but that would be a double check to whatever checks are on dependencies... And also a huge work to check all these panics. Here we are talking about simple bounds or dimension checks but there can be cases where the dependency does some huge calculation and panics based on the results...

yutannihilation commented 5 months ago

So, your struggle is mainly about diagnosing the problem, right? Then, I think the debug mode will save you.

daniellga commented 5 months ago

It's actually about making the user not lose his R session when a panic occurs because of a mistake in one of the arguments.

daniellga commented 5 months ago

could you please briefly ELI5 me why R crashes when a panic occurs? Do you know if that's done the same way in PyO3?

yutannihilation commented 5 months ago

You know, panic means unrecoverable, catastrophic error. I'm strongly against providing the false sense of safety to your users by recovering the unrecoverables.

https://doc.rust-lang.org/book/ch09-01-unrecoverable-errors-with-panic.html

daniellga commented 5 months ago

Is there any way to make it opt in? Like in a feature? Rust is full of hidden panics like in bounds checking, division by zero and others...

yutannihilation commented 5 months ago

Do you know if that's done the same way in PyO3?

I don't use PyO3 by myself, but it seems PyO3 treats panic as a type of Exception.

https://stackoverflow.com/questions/76377350/pyo3-and-panics https://pyo3.rs/main/doc/pyo3/panic/struct.panicexception

Please be aware that the difference comes from the fact the error-handling mechanism of R is actually different from Python in that R uses longjmp, which is considered UB when it crosses over FFI.

Is there any way to make it opt in? Like in a feature?

I'm sorry, no plan at the moment. Again, I'm strongly against providing the false sense of safety. While I understand your struggle, since this is the very main reason I needed to create savvy, I don't get easily convinced. Extendr keeps relying on (possible) undefined behavior and there seems no one who can fix it.

c.f. https://github.com/extendr/extendr/issues/278#issuecomment-1025645872

If you think some operation of your third-party crate is fallible in nature, you should ask the author to return Result. Or, if you are sure it's safe to catch the panic, I don't stop you using catch_unwind().

yutannihilation commented 5 months ago

Let me be a bit more clear about catch_unwind(). As long as unwinding the stack of Rust's side, it's not undefined behavior. So, while it's not recommended to use this as a general try/catch mechanism, it is probably safe. However, as the framework calls R API, it's inappropriate to wrap all the Rust code by the framework's side.

daniellga commented 5 months ago

I think catch_unwind will be the way to go. ndarray is too big of a crate for me alone to improve their error handling. Or let it crash R and warn in documentation the reasons for the crash. If I understood correctly, PyO3 seems to behave the same way as savvy, causing the interpreter to exit.

yutannihilation commented 5 months ago

If I understood correctly, PyO3 seems to behave the same way as savvy, causing the interpreter to exit.

Oh, good to know. Thanks.

yutannihilation commented 5 months ago

(Sorry for a drifted topic)

Reading the discussion on PyO3, it seems I understand the problem oppositely (?). It needs to be abort the session anyway in the case of panic, but, at the same time, the unwinding should be stopped before the FFI boundary.

yutannihilation commented 5 months ago

If I understood correctly, PyO3 seems to behave the same way as savvy, causing the interpreter to exit.

I tried some polars Python code that are reported to cause panics in their repository, but it seems it doesn't exit the interpreter.

daniellga commented 5 months ago

could you link here the functions you tried?

yutannihilation commented 5 months ago

You can search.

https://github.com/search?q=repo%3Apola-rs%2Fpolars+panic&type=issues

daniellga commented 5 months ago

It's weird that PyO3 docs said it should exit the session... I searched for catch_unwind in polars code but didn't find any... I don't know what's going on...

daniellga commented 5 months ago

https://github.com/PyO3/pyo3/issues/3519#issuecomment-1775982134

it seems polars chose to expose PanicException to the user.

yutannihilation commented 5 months ago

Oh, thanks. Their direction sounds reasonable, although it seems abort was not adopted in actual. (I did try panic=abort mainly for the binary size)

Btw, thinking this problem again, it seems there's no cross-language unwinding or cross-language longjmp, so I'm wondering it's a valid choice to catch panic.

https://github.com/yutannihilation/savvy/issues/164

Sorry I think I'm still in confusion. This was the gamechanger, and I need some more time to understand the current status.

https://yutani.rbind.io/post/dont-panic-we-can-unwind/

daniellga commented 5 months ago

No problem. You are definitely understanding way more than I am. Take your time.

yutannihilation commented 5 months ago

@daniellga Hi, I still cannot decide which, but now the dev build (when you build with devtools::load_all(), or set DEBUG envvar to "true"), panic doesn't crash your session. If you have time, could you try if this helps you? This version is not released yet, so you need to specify savvy = { git = "https://github.com/yutannihilation/savvy" } to use this feature. (edit: this is now published as version 0.5.2)

daniellga commented 5 months ago

@yutannihilation thanks! I will give it a look when possible. The panic=unwind solution should do it. I believe this behaviour should be in your guide since it's pretty common for rust to panic in many situations.

yutannihilation commented 5 months ago

Thanks, I do rewrite the guide once the things are settled. I'm planning to make this behavior available also on release build as an opt-in feature (#186), but I'm still thinking. Anyway, I'd appreciate your feedback.

yutannihilation commented 5 months ago

I wrote a document about how to deal with panic!. If you feel something is missing here, please let me know :)

https://yutannihilation.github.io/savvy/guide/error.html#dealing-with-panic