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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6445528
(wip) try_catch
kjvalencik Jun 18, 2020
f608981
JsResultExt
kjvalencik Jun 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/neon-runtime/src/nan/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,4 @@ pub mod convert;
pub mod class;
pub mod task;
pub mod handler;
pub mod try_catch;
11 changes: 11 additions & 0 deletions crates/neon-runtime/src/nan/try_catch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/// Creates a `Nan::TryCatch`
pub use neon_sys::Neon_TryCatch_New as new;

/// Returns a boolean indicating if an exception has been caught
pub use neon_sys::Neon_TryCatch_HasCaught as has_caught;

/// Returns an exception if one has been caught
pub use neon_sys::Neon_TryCatch_Exception as exception;

/// Deletes an instance of `Nan::TryCatch`
pub use neon_sys::Neon_TryCatch_Delete as delete;
1 change: 1 addition & 0 deletions crates/neon-sys/native/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
{
"main": "index.js",
"scripts": {
"node": "node",
"preinstall": "echo 'Skipping node-gyp installation as part of npm install.'",
"configure-release": "node-gyp configure --verbose",
"configure-debug": "node-gyp configure --verbose --debug",
Expand Down
16 changes: 16 additions & 0 deletions crates/neon-sys/native/src/neon.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

}

extern "C" bool Neon_TryCatch_HasCaught(Nan::TryCatch *try_catch) {
return try_catch->HasCaught();
}

extern "C" void* Neon_TryCatch_Exception(Nan::TryCatch *try_catch) {
return *try_catch->Exception();
}

extern "C" void Neon_TryCatch_Delete(Nan::TryCatch *try_catch) {
try_catch->~TryCatch();
}
5 changes: 5 additions & 0 deletions crates/neon-sys/native/src/neon.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,11 @@ extern "C" {
void* Neon_EventHandler_New(v8::Isolate *isolate, v8::Local<v8::Value> self, v8::Local<v8::Function> callback);
void Neon_EventHandler_Schedule(void* thread_safe_cb, void* rust_callback, Neon_EventHandler handler);
void Neon_EventHandler_Delete(void* thread_safe_cb);

void* Neon_TryCatch_New();
bool Neon_TryCatch_HasCaught(Nan::TryCatch *try_catch);
void* Neon_TryCatch_Exception(Nan::TryCatch *try_catch);
void Neon_TryCatch_Delete(Nan::TryCatch *try_catch);
}

#endif
5 changes: 5 additions & 0 deletions crates/neon-sys/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,4 +202,9 @@ extern "C" {
pub fn Neon_EventHandler_Schedule(thread_safe_cb: *mut c_void, rust_callback: *mut c_void,
complete: unsafe extern fn(Local, Local, *mut c_void));
pub fn Neon_EventHandler_Delete(thread_safe_cb: *mut c_void);

pub fn Neon_TryCatch_New() -> *mut c_void;
pub fn Neon_TryCatch_HasCaught(try_catch: *mut c_void) -> bool;
pub fn Neon_TryCatch_Exception(try_catch: *mut c_void) -> Local;
pub fn Neon_TryCatch_Delete(try_catch: *mut c_void);
}
25 changes: 25 additions & 0 deletions src/context/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,31 @@ pub trait Context<'a>: ContextInternal<'a> {
let err = JsError::range_error(self, msg)?;
self.throw(err)
}

fn try_catch<T, F>(&mut self, f: F) -> Result<Handle<'a, T>, Handle<'a, JsValue>>
where
T: Value,
F: FnOnce(&mut Self) -> JsResult<'a, T>
{
let try_catch = unsafe { neon_runtime::try_catch::new() };
let result = match f(self) {
Ok(v) => Ok(v),
Err(Throw) => {
assert!(
unsafe { neon_runtime::try_catch::has_caught(try_catch) },
"Expected VM to be in a throwing state",
);

let err = unsafe { neon_runtime::try_catch::exception(try_catch) };

Err(Handle::new_internal(JsValue::from_raw(err)))
},
};

unsafe { neon_runtime::try_catch::delete(try_catch) };

result
}
}

/// A view of the JS engine in the context of top-level initialization of a Neon module.
Expand Down
13 changes: 13 additions & 0 deletions src/result/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

where
V: Value,
E: Value,
{
fn or_throw<'b, C: Context<'b>>(self, cx: &mut C) -> JsResult<'a, V> {
match self {
Ok(v) => Ok(v),
Err(err) => cx.throw(err),
}
}
}
9 changes: 9 additions & 0 deletions test/dynamic/native/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,14 @@ use js::classes::*;
use js::tasks::*;
use js::eventhandler::*;

pub fn catch_exception(mut cx: FunctionContext) -> JsResult<JsValue> {
Ok(cx.try_catch(|cx| {
let _ = cx.throw_error("Something bad happened")?;

Ok(cx.string("Unreachable").upcast())
}).unwrap_or_else(|err| err))
}

register_module!(mut cx, {
cx.export_function("return_js_string", return_js_string)?;

Expand Down Expand Up @@ -70,6 +78,7 @@ register_module!(mut cx, {

cx.export_function("panic", panic)?;
cx.export_function("panic_after_throw", panic_after_throw)?;
cx.export_function("catch_exception", catch_exception)?;

cx.export_class::<JsEmitter>("Emitter")?;
cx.export_class::<JsTestEmitter>("TestEmitter")?;
Expand Down