Migrated error type to napi error #505

anshulrgoyal · 2020-03-25T13:24:39Z

Description

This pull request try to migrate nan error management to napi implementation using nodejs-sys as backend. The napi provide few methods for error which are documented below:

napi_throw this throws an error.
napi_create_error this create a new error.
napi_create_type_error this create a new type error.
napi_create_range_error this create a new range error.

Related Issue

#444

anshulrgoyal · 2020-03-25T13:28:44Z

@dherman There is a problem with handling panic!(error) all the napi function require napi_env which is not available in the panic_hook.

kjvalencik

Thanks for contributing to neon! Let me know if you have any questions on my comments.

kjvalencik · 2020-03-25T16:59:24Z

crates/neon-runtime/src/napi/error.rs

-pub unsafe extern "C" fn new_error(_out: &mut Local, _msg: Local) { unimplemented!() }
+pub unsafe extern "C" fn throw(env:Env,error: Local)->bool {
+    let status=napi::napi_throw(env,error);
+    status==napi::napi_status::napi_ok


#Fix: This should assert napi_ok similar to other methods instead of returning a boolean. The Rust code is not checking the result.

Let's still remove the bool return type from this function as @kjvalencik suggests. Once you've asserted that status is napi_ok it's not helpful to test that comparison a second time and then ignore the result anyway.

kjvalencik · 2020-03-25T17:00:33Z

.gitignore

@@ -10,3 +10,5 @@ cli/lib
 test/cli/lib
 npm-debug.log
 rls*.log
+.idea


#Fix: Remove this line. I recommend creating a global git ignore.

https://help.github.com/en/github/using-git/ignoring-files#configuring-ignored-files-for-all-repositories-on-your-computer

kjvalencik · 2020-03-25T17:01:02Z

crates/neon-runtime/src/napi/error.rs


-pub unsafe extern "C" fn new_type_error(_out: &mut Local, _msg: Local) { unimplemented!() }
+pub unsafe extern "C" fn new_error(out: &mut Local,env:Env,code:Local,msg:Local)->bool {


#Fix: Please follow formatting of other functions.

kjvalencik · 2020-03-25T17:01:20Z

src/context/mod.rs

@@ -115,22 +115,22 @@ impl<'a> Lock<'a> {
 }

 /// An _execution context_, which provides context-sensitive access to the JavaScript engine. Most operations that interact with the engine require passing a reference to a context.
-/// 
+///


#Fix: Please remove unrelated formatting changes.

kjvalencik · 2020-03-25T17:02:22Z

src/types/error.rs

@@ -36,6 +36,7 @@ impl Object for JsError { }

 impl JsError {
    /// Creates a direct instance of the [`Error`](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Error) class.
+    #[cfg(feature = "legacy-runtime")]


#Answer: Since these functions share a function signature, what do you think of putting the cfg flag on the body instead of on the function to ensure conformance?

kjvalencik · 2020-03-25T17:04:18Z

src/types/error.rs

@@ -44,7 +45,23 @@ impl JsError {
        })
    }

+    #[cfg(feature = "napi-runtime")]
+    pub fn error<'a, C: Context<'a>, S: AsRef<str>>(cx: &mut C, msg: S) -> NeonResult<Handle<'a, JsError>> {
+        let (ptr, len) = if let Some(small) = Utf8::from(msg.as_ref()).into_small() {


Are we able to re-use the JsString code for this?

JsString will create a struct and then i have to use to_raw on it is not ideal according to me

anshulrgoyal · 2020-03-25T20:52:37Z

@kjvalencik I am still stuck with panic hook problem. So the problem is that all napi function require napi_env as there first argument and since it is not available in panic hook i am not able to implement it properly.

kjvalencik · 2020-03-25T21:15:09Z

~~Which piece of code are you referring to as the panic hook problem? Do you mean convert_panics?~~

I see the problem convert_panics requires an env, but, it was consumed by the call. We could potentially alias the raw pointer to napi_env via to_raw(), but, I would need to think about the implications and if that's safe.

I think at the very least it's not any less safe because that's what Isolate::current() is implicitly doing.

anshulrgoyal · 2020-04-13T12:20:13Z

@kjvalencik would you moving forward with this pr?

anshulrgoyal · 2020-04-18T12:59:14Z

~~Which piece of code are you referring to as the panic hook problem? Do you mean convert_panics?~~

I see the problem convert_panics requires an env, but, it was consumed by the call. We could potentially alias the raw pointer to napi_env via to_raw(), but, I would need to think about the implications and if that's safe.

I think at the very least it's not any less safe because that's what Isolate::current() is implicitly doing.

I donot think aliasing will cause a problem since the napi_env is valid till the addon is mounted.

dherman

Lots of good stuff in here, thanks so much! I have a few more requested changes. Thanks!

dherman · 2020-05-16T23:02:35Z

crates/neon-runtime/src/napi/error.rs

-pub unsafe extern "C" fn new_error(_out: &mut Local, _msg: Local) { unimplemented!() }
+pub unsafe extern "C" fn throw(env:Env,error: Local)->bool {
+    let status=napi::napi_throw(env,error);
+    status==napi::napi_status::napi_ok


Let's still remove the bool return type from this function as @kjvalencik suggests. Once you've asserted that status is napi_ok it's not helpful to test that comparison a second time and then ignore the result anyway.

crates/neon-runtime/src/napi/error.rs

src/types/error.rs

crates/neon-runtime/src/napi/error.rs

src/object/class/internal.rs

src/types/error.rs

kjvalencik · 2020-07-07T21:26:33Z

@anshulrgoyal Thanks so much for your contribution! I'm sorry that I let this fall behind. I took the liberty of picking up your PR, resolving conflicts, and closing out the last couple of remaining questions.

Thanks so much! #542

Migrated error type to napi error

8004779

Error error during legacy runtime build

4ab5356

kjvalencik requested changes Mar 25, 2020

View reviewed changes

Anshul Goyal added 5 commits March 26, 2020 02:48

Removed formating

2294f55

Removed formating

cee21d1

REmoved more formating

393139a

added cfg to function bodies

d710798

Fixed error

d18c514

Fixed panic hook

126eaf8

Fixed build on stable version

48ef51c

dherman requested changes May 16, 2020

View reviewed changes

Anshul Goyal and others added 5 commits May 17, 2020 11:21

Removed duplicate code

4b5d4d7

formatting fix

6d654ec

Update internal.rs

bf74b25

Merge branch 'master' into master

4be3c5c

Fixed double import

cabb576

anshulrgoyal requested review from dherman and kjvalencik May 31, 2020 13:57

kjvalencik closed this Jul 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrated error type to napi error #505

Migrated error type to napi error #505

anshulrgoyal commented Mar 25, 2020

anshulrgoyal commented Mar 25, 2020

kjvalencik left a comment

kjvalencik Mar 25, 2020

anshulrgoyal Mar 26, 2020

dherman May 16, 2020

kjvalencik Mar 25, 2020

anshulrgoyal Mar 26, 2020

kjvalencik Mar 25, 2020

anshulrgoyal Mar 26, 2020

kjvalencik Mar 25, 2020

anshulrgoyal Mar 26, 2020

kjvalencik Mar 25, 2020

anshulrgoyal Mar 26, 2020

kjvalencik Mar 25, 2020

anshulrgoyal Mar 26, 2020

anshulrgoyal commented Mar 25, 2020

kjvalencik commented Mar 25, 2020 •

edited

Loading

anshulrgoyal commented Apr 13, 2020

anshulrgoyal commented Apr 18, 2020

dherman left a comment

dherman May 16, 2020

kjvalencik commented Jul 7, 2020 •

edited

Loading


		pub unsafe extern "C" fn new_type_error(_out: &mut Local, _msg: Local) { unimplemented!() }
		pub unsafe extern "C" fn new_error(out: &mut Local,env:Env,code:Local,msg:Local)->bool {

Migrated error type to napi error #505

Migrated error type to napi error #505

Conversation

anshulrgoyal commented Mar 25, 2020

Description

Related Issue

anshulrgoyal commented Mar 25, 2020

kjvalencik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anshulrgoyal commented Mar 25, 2020

kjvalencik commented Mar 25, 2020 • edited Loading

anshulrgoyal commented Apr 13, 2020

anshulrgoyal commented Apr 18, 2020

dherman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjvalencik commented Jul 7, 2020 • edited Loading

kjvalencik commented Mar 25, 2020 •

edited

Loading

kjvalencik commented Jul 7, 2020 •

edited

Loading