Propagate errors instead of panics

[cgrigis]

See #47

Replace some unwrap() or expect() calls with error propagation or anyhow::context().

This fixes the main ones I could find. Some others remain, in the following categories:

• Cannot be triggered AFAIU, e.g.:
  • https://github.com/c4dt/arti-rest/blob/main/src/client.rs#L109 (will panic on partial response, but this is handled before in https://github.com/c4dt/arti-rest/blob/main/src/client.rs#L94-L96)
  • https://github.com/c4dt/arti-rest/blob/main/src/arti/tor-dirmgr/src/state.rs#L209 (will panic on None, but explicitely assigned to Some(...) in https://github.com/c4dt/arti-rest/blob/main/src/arti/tor-dirmgr/src/state.rs#L199-L207)
• Tests
• Deeply rooted into Arti code (e.g. shared_ref.rs, config.rs) and might require some more time to address, besides possibly conflicting with Arti's code evolution in //.

Should I work on these last ones?

[ineiti]

I'm OK with doing the catch_unwind that @ubamrein proposed for the rest of the panics...

[ineiti]

@laurent-girod I didn't follow where these files come from. Is the same thing happening in the original arti-code, too? Then we could create a PR against their code.

[cgrigis]

I added a catch_unwind() around the main client tls_send() call. In my tests on Android, that seems to catch panics that occur underneath.

NOTE: I used a match on the returned value, because when I tried to use a context() directly on it, the compiler complains with a trait bound not satisfied which is not clear to me how to fix.

[ineiti]

That's weird for me to use a catch_unwind not at leaf code. If one wants to provide arti also as a rust library, handling panic at all is bad practice.

From the documentation https://doc.rust-lang.org/std/panic/fn.catch_unwind.html:

this function is particularly useful when Rust is called from another language

which is what we're doing here, no?

[ubamrein]

That's weird for me to use a catch_unwind not at leaf code. If one wants to provide arti also as a rust library, handling panic at all is bad practice.

From the documentation https://doc.rust-lang.org/std/panic/fn.catch_unwind.html:

this function is particularly useful when Rust is called from another language

which is what we're doing here, no?

When I understood you correctly, you put the catch_unwind in the ffi code right? Because then it should be fine, since there is no other way of handling a panic from java/swift.

[ubamrein]

Sorry for that, wanted to cancel my draft comment...

[ineiti]

When I understood you correctly, you put the catch_unwind in the ffi code right?

Nearly in the ffi code: It's the android- and ios-ffi code that call out to Client.send, and this is where the catch_unwind is seated.

[cgrigis]

I first put it directly in the FFI code, but then moved it to the client code used by both FFI's (which in turn calls Arti), so as to concentrate the change in a single point. If it is more appropriate, I will move it back to the FFI.

[ubamrein]

I first put it directly in the FFI code, but then moved it to the client code used by both FFI's (which in turn calls Arti), so as to concentrate the change in a single point. If it is more appropriate, I will move it back to the FFI.

I mean as @tharvik said, it is considered bad practice to catch panics like that, except for inter language API design... So I'd rather go for a catch_unwind in the ios FFI and android FFI code.

[cgrigis]

Moved the catch_unwind() to the Android FFI code. @ineiti I will let you do the iOS version, as I don't think I can easily test that on my system.

[ineiti]

I mean as @tharvik said, it is considered bad practice to catch panics like that, except for inter language API design... So I'd rather go for a catch_unwind in the ios FFI and android FFI code.

Client is only ever used from the FFI - it's the common part of it, and could/should get some more of the duplicated code in android/ios ffi. Pushing unwind to the actual ffi-code is only duplication.

Anyway, here is a PR for the iOS part: #52

[cgrigis]

My opinion:

As I stated, avoiding duplication was my reason to put the code in the Client code at some point.

After seeing @tharvik and @ubamrein's point though, I have to agree: the panic-catching code is something we're doing because of foreign code and should be done at its boundary, whereas Client is arguably part of the library itself, which even if not currently used by code other than FFI, can be envisioned to be used directly by a Rust lib user. Sometimes architecture cleanliness trumps (minor) duplication. ;-)

[ineiti]

I'm merging this while I wait for @laurent-girod 's wisdom to help me get the test-lib pass in #52 ...