Neon: Feature request: loosen try_catch's type to return Result<_, Handle<'a, JsValue>>

[jrose-signal]

Context::try_catch's current signature is

fn try_catch<'b: 'a, T, F>(
    &mut self,
    f: F
) -> Result<Handle<'a, T>, Handle<'a, JsValue>>
where
    T: Value,
    F: UnwindSafe + FnOnce(&mut Self) -> JsResult<'b, T>, 

However, nothing in the implementation (as far as I can tell) actually relies on the return value being a JS value. It could just as easily be

fn try_catch<'b: 'a, T, F>(
    &mut self,
    f: F
) -> Result<T, Handle<'a, JsValue>>
where
    F: UnwindSafe + FnOnce(&mut Self) -> NeonResult<T>, 

[kjvalencik]

@jrose-signal I like this idea and it would be good for the N-API backend, but unfortunately it would be complicated for the legacy backend.

v8::TryCatch need to be allocated on the stack and this forces Rust to execute from C++. The returned data would need to be heap allocated and Send. This still might be valuable and worth it. I'm open to the idea since we haven't stabilized the API yet.

Can you share more about your use case? In my opinion, try_catch blocks should be fairly tightly scoped. Is there something in Neon's API that returns a T but, can also throw that you would like to catch?

Thanks!

[jrose-signal]

v8::TryCatch need to be allocated on the stack and this forces Rust to execute from C++. The returned data would need to be heap allocated and Send.

I think it just has to be Sized: you've already got a MaybeInit here

https://github.com/neon-bindings/neon/blob/1f217f56cb2a1760621e76c1dc317526229de3b3/src/context/internal.rs#L137

that gets initialized from pure Rust code here

https://github.com/neon-bindings/neon/blob/1f217f56cb2a1760621e76c1dc317526229de3b3/src/context/internal.rs#L208

without C++ doing anything more than passing the pointer down the stack. That should be safe to treat as a mutable borrow, yeah?

Can you share more about your use case? In my opinion, try_catch blocks should be fairly tightly scoped. Is there something in Neon's API that returns a T but, can also throw that you would like to catch?

Mine's not a tightly-scoped use case, I'm afraid: I'm trying to deal with a Rust-side function throwing during the fulfillment of a JavaScript promise, which older versions of Node just drop on the floor. For now I'm just making a dummy undefined value, but it seems like I ought to be able to wrap any part of my Rust logic, whether it produces a JavaScript result, a Rust result, or no result at all (well, ()). Think of it like catch_unwind.

(I've actually made things much harder for myself in the real scenario, but I won't get into that here.)

[kjvalencik]

That value is used for both the Ok and Err result and a separate discriminator is used. Instead, two separate pointers would be needed. It's a little more complex, but I do like it.

@dherman What are your thoughts? It does seem like a strictly _better_ API, even if it makes the legacy backend more complicated.

[kjvalencik]

@jrose-signal I put a quick PoC together for this, but I need to think a little more about the correctness of only specifying Sized or if Send should be included as well (I don't think it needs to be) and if this needs to be boxed to ensure the pointer size. https://github.com/neon-bindings/neon/pull/631

[dherman]

Looks good to me! It's a backwards-compatible change from the legacy backend type to the N-API backend type, so I don't foresee it causing problems for the two backends to have different return types.

[jrose-signal]

Come to think of it, F doesn't have to be UnwindSafe either even in the legacy runtime, because try_catch immediately resumes the unwind on the other side of the FFI boundary.

[kjvalencik]

@jrose-signal if it's not unwind safe, how would we keep the panic from propagating over the FFI boundary?

As an aside, I think stack unwinding through C++ is handled by Rust even though it's not officially supported.

[jrose-signal]

I'll admit I've had trouble fully understanding UnwindSafe, but this line from the documentation seems to imply that it'd be okay here:

Simply put, a type T implements UnwindSafe if it cannot easily allow witnessing a broken invariant through the use of catch_unwind (catching a panic).

In this case, your use of catch_unwind would not allow witnessing a broken invariant, because it is caught right below the FFI boundary and resumed right above, with only the box for the unwind info manipulated in between. So even if F has problems if someone uses catch_unwind higher up the stack, it'd still be appropriate to use AssertUnwindSafe for this specific use within try_catch.

[kjvalencik]

🤔 That's a good point. Since we do not execute any of the Neon user's code after catching a panic, we should be okay only verifying Neon's invariants.

I'll take this under advisement. We may choose to keep it for now since relaxing a trait bound in a public API is easier than adding one.

[dherman]

I checked with Niko Matsakis and he reassured me that if we re-raise the panic, we can AssertUnwindSafe and don't need to pass the trait bound on to the external API.

[dherman]

(I think in mathematics that was what they call "proof by core team")

[kjvalencik]

Closed in https://github.com/neon-bindings/neon/pull/631