Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(wip) try_catch #533

Closed
wants to merge 2 commits into from
Closed

(wip) try_catch #533

wants to merge 2 commits into from

Conversation

kjvalencik
Copy link
Member

Implementation of neon-bindings/rfcs#29

@@ -541,3 +541,19 @@ extern "C" void Neon_EventHandler_Delete(void * thread_safe_cb) {
neon::EventHandler *cb = static_cast<neon::EventHandler*>(thread_safe_cb);
cb->close();
}

extern "C" void* Neon_TryCatch_New() {
return new Nan::TryCatch();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this isn't going to work. :( a TryCatch has to be allocated on the stack. There are a couple ways to do this, both of which are pretty annoying:

  • Use a cross-language callback pattern, where you pass the Rust closure as a void* to C along with a Rust glue function that takes the Rust closure as an argument and calls in. Then in C you stack-allocate the TryCatch and invoke the Rust glue function.
  • Allocate a TryCatch on the Rust stack by creating a parallel Rust struct type that's big enough to contain the sizeof the C++ TryCatch. For this you have to have some way at build time to determine the sizeof a TryCatch and use a macro or a const declaration to declare the Rust type. And then the constructor and destructor for that Rust type can call into neon_runtime with a pointer to self for the C++ code to explicitly call the C++ constructor and destructor on it. This is how HandleScope works, which also has to be stack allocated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that the reason they should be stack allocated is for nesting to work properly they need to be deallocated in the same order they were created.

RII also enforces this. I think deleting with Drop would also accomplish this.

With that said, I think your suggestion of passing the Rust closure to be invoked by C is best.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, panicking across FFI is undefined.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this is neat. rust-lang/rust#65646

@@ -31,3 +31,16 @@ pub type JsResult<'b, T> = NeonResult<Handle<'b, T>>;
pub trait JsResultExt<'a, V: Value> {
fn or_throw<'b, C: Context<'b>>(self, cx: &mut C) -> JsResult<'a, V>;
}

impl <'a, V, E> JsResultExt<'a, V> for Result<Handle<'a, V>, Handle<'a, E>>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand the use case for this. If I wrap something in a try_catch it's to prevent it from throwing. If I want to do something like map on a result that might have thrown, I already have a Result that I can do map() on and it'll throw.

Is this for cases where you want to map and do something back in the JS VM? Like:

cx.try_catch(do_something)
  .and_then(|v| {
      ... cx ... // do stuff in the JS VM
  })
  .or_throw(cx)

@kjvalencik kjvalencik closed this Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants