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

Add logic to handle graceful error handling when primitive errors are thrown by JS functions #1075

Merged
merged 7 commits into from
Nov 26, 2021

Conversation

JckXia
Copy link
Member

@JckXia JckXia commented Sep 21, 2021

Added some logic to override Value() function on the Error object to do the unwrapping, as well wrapping logic to the Object(env, value) function accordingly.

napi-inl.h Outdated
// We are checking if the object is wrapped
bool isWrappedObject = false;
napi_has_property(
_env, refValue, String::From(_env, "isWrapObject"), &isWrappedObject);
Copy link
Member

Choose a reason for hiding this comment

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

I think we likely want to make the "isWrapObject" more unique to avoid potential collision with user code. Maybe use a GUID with -isWrappedObject tacked on to the end?

@@ -14,6 +14,10 @@ If C++ exceptions are enabled (for more info see: [Setup](setup.md)), then the
`Napi::Error` class extends `std::exception` and enables integrated
error-handling for C++ exceptions and JavaScript exceptions.

Note, that due to limitations of the N-API, if one attempt to cast the error object thrown as a primitive, an
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Note, that due to limitations of the N-API, if one attempt to cast the error object thrown as a primitive, an
Note, that due to limitations of the N-API, if one attempts to cast the error object thrown as a primitive, an

@@ -14,6 +14,10 @@ If C++ exceptions are enabled (for more info see: [Setup](setup.md)), then the
`Napi::Error` class extends `std::exception` and enables integrated
error-handling for C++ exceptions and JavaScript exceptions.

Note, that due to limitations of the N-API, if one attempt to cast the error object thrown as a primitive, an
wrapped object will be received instead. (With properties ```isWrapObject``` and ```errorVal``` containing the primitive value thrown)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just add some additional clarification that this is when the node-addon-api Error C++ object is used within the addon.

// Avoid infinite recursion in the failure case.
// Don't try to construct & throw another Error instance.
NAPI_FATAL_IF_FAILED(status, "Error::Error", "napi_create_reference");
}
}

inline Object Error::Value() const {
Copy link
Member

Choose a reason for hiding this comment

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

I think it might make sense for this to return Value instead of Object (Values can be objects), the main concern is that would potentially be breaking.

The reason is that we wrapping so that we can have a value versus just objects. If its possible we might want two methods one which returns a Value and one which returns an Object in order to avoid breaking existing code.

Copy link
Member Author

@JckXia JckXia Sep 21, 2021

Choose a reason for hiding this comment

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

I tried setting the return type to Value and it seems to pass all the test suites. Although I am not too sure if that breaks any conventions. (i.e, In the base Reference class, Reference<T>::Value() will return an instance of type T, and Error is of type Object since it inherits ObjectReference, which it self inherits from Reference<Object>. Therefore people might expect Error::Value() to return an Object also)

For the second part then, can I declare two public methods Napi::Value GetUnWrappedValue() and Object Value() ?

Copy link
Member

Choose a reason for hiding this comment

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

If is possible to have 2 methods both called Value() which return different types. If so that might make sense assuming the compiler will select the most appropriate one based on how you use the return value?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I am not too sure if it's possible to overload base on return value in C++ (without using templates) according to this post (https://stackoverflow.com/questions/9568852/overloading-by-return-type).

Copy link
Member

Choose a reason for hiding this comment

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

After discussing in Node-API team meeting I think this is ok as is.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

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

Just some notes about documentation.

@@ -14,6 +14,10 @@ If C++ exceptions are enabled (for more info see: [Setup](setup.md)), then the
`Napi::Error` class extends `std::exception` and enables integrated
error-handling for C++ exceptions and JavaScript exceptions.

Note, that due to limitations of the N-API, if one attempts to cast the error object wrapping a primitive inside a C++ addon, the wrapped object
Copy link
Member

Choose a reason for hiding this comment

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

Node-API instead of N-API

@@ -14,6 +14,10 @@ If C++ exceptions are enabled (for more info see: [Setup](setup.md)), then the
`Napi::Error` class extends `std::exception` and enables integrated
error-handling for C++ exceptions and JavaScript exceptions.

Note, that due to limitations of the N-API, if one attempts to cast the error object wrapping a primitive inside a C++ addon, the wrapped object
will be received instead. (With properties ```4b3d96fd-fb87-4951-a979-eb4f9d2f2ce9-isWrapObject``` and ```errorVal``` containing the primitive value thrown)
Copy link
Member

Choose a reason for hiding this comment

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

It's inline code so please use one back tick es. 4b3d96fd-fb87-4951-a979-eb4f9d2f2ce9-isWrapObject

doc/error_handling.md Show resolved Hide resolved
napi-inl.h Show resolved Hide resolved
napi-inl.h Outdated

// We are checking if the object is wrapped
bool isWrappedObject = false;
napi_has_property(
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a typecheck first to see that it's not a Symbol. If the reference is a Symbol it's definitely not wrapped. In new Node.js versions, the reference can be a symbol.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Though do we need to consider the cases where people are using an older Node.js version?

napi.h Outdated
@@ -8,7 +8,6 @@
#include <mutex>
#include <string>
#include <vector>

Copy link
Contributor

Choose a reason for hiding this comment

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

This line should remain.

@@ -31,6 +31,7 @@ Object InitDate(Env env);
Object InitDataView(Env env);
Object InitDataViewReadWrite(Env env);
Object InitEnvCleanup(Env env);
Object InitErrorHandlingPrim(Env env);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these names consistent with the file names being used? This will help @deepakrkris with the unit test filtering.

Copy link
Contributor

Choose a reason for hiding this comment

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

@deepakrkris can you double-check this please?

Napi::Object exports = Napi::Object::New(env);
exports.Set("errorHandlingPrim", Napi::Function::New<Test>(env));
return exports;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a newline to the end of the file!

Copy link
Contributor

@KevinEady KevinEady left a comment

Choose a reason for hiding this comment

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

See comments

napi-inl.h Outdated
Comment on lines 2594 to 2600
status = napi_set_property(env,
wrappedErrorObj,
String::From(env, "errorVal"),
Value::From(env, value));
NAPI_FATAL_IF_FAILED(status, "Error::Error", "napi_set_property");

status = napi_set_property(
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC using napi_set_property will allow the property to be enumerable and writable. Maybe we should use napi_define_properties and make the property read-only? What about enumerable?

napi-inl.h Outdated
env,
wrappedErrorObj,
String::From(env,
"4b3d96fd-fb87-4951-a979-eb4f9d2f2ce9-isWrapObject"),
Copy link
Contributor

Choose a reason for hiding this comment

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

This magic-value, can we make as some constant? Maybe a private static const on Error class? Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

@devsnek 's suggestion is better: use a symbol instead of this string.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @KevinEady. I updated my PR to opt using symbol instead of this magic string but ran into some issues.

In particular, when I ran the tests, the test/error_terminating_environment.js suite will crash consistently. (By this command test(`./build/${buildType}/binding_swallowexcept.node`, false);. Looking at the code I am assuming it is saying that by running the tests in this configuration, the process shouldn't crash but be handled gracefully.

However, when I extract the error message from the spawnSync call, it looks like somewhere, an Error instance was thrown and crashed the app, but I can't quite pinpoint where exactly.

workerFailure

After doing some debugging, all I can find is that if I remove the binding.error.waitForWorkerThread() function statement on line 32 of the error_terminating_environment.js file, no Error is thrown but I am not quite sure if the tests are ran either. Any ideas on why this might be happening? Thanks!

napi-inl.h Outdated
env,
wrappedErrorObj,
String::From(env,
"4b3d96fd-fb87-4951-a979-eb4f9d2f2ce9-isWrapObject"),
Copy link
Member

Choose a reason for hiding this comment

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

can a symbol not be used here? seems like the perfect use case.

napi-inl.h Outdated
String::From(_env, "4b3d96fd-fb87-4951-a979-eb4f9d2f2ce9-isWrapObject"),
&isWrappedObject);

MaybeOrValue<Symbol> result = Symbol::For(_env, "isWrapObject");
Copy link
Member

Choose a reason for hiding this comment

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

One suggestion is maybe we should still use a GUID here instead of the string.

napi-inl.h Outdated

napi_property_descriptor errValDesc = {
"errorVal", // const char* utf8Name
String::From(env, "errorVal"), // napi_value name
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be nullptr. utf8name is sufficient for specifying the name of the property.

napi-inl.h Outdated
nullptr};

#ifdef NODE_ADDON_API_ENABLE_MAYBE
Symbol uniqueSymb;
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Move this declaration outside the #ifdef.

napi-inl.h Outdated
NAPI_FATAL_IF_FAILED(status, "Error::Error", "napi_define_properties");
#else

wrapObjFlag.name = result;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
wrapObjFlag.name = result;
uniqueSymb = result;
wrapObjFlag.name = result;

napi-inl.h Outdated
Comment on lines +2641 to +2642
status = napi_define_properties(env, wrappedErrorObj, 1, &wrapObjFlag);
NAPI_FATAL_IF_FAILED(status, "Error::Error", "napi_define_properties");
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Move these two statements below the #endif. That way, you are not duplicating them in both bodies of the #if...#else...#endif, thereby reducing the size of the the #if...#else...#endif block.

Comment on lines 2636 to 2637
status = napi_define_properties(env, wrappedErrorObj, 1, &wrapObjFlag);
NAPI_FATAL_IF_FAILED(status, "Error::Error", "napi_define_properties");
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Remove these two lines.

napi-inl.h Outdated
// We are checking if the object is wrapped
bool isWrappedObject = false;

MaybeOrValue<Symbol> result = Symbol::For(_env, "isWrapObject");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also be guarded by #ifdef NODE_ADDON_API_ENABLE_MAYBE since it's MaybeOrValue<Symbol>?

napi-inl.h Outdated
if (result.IsJust()) {
uniqueSymb = result.Unwrap();
}
status = napi_has_property(_env, refValue, uniqueSymb, &isWrappedObject);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the same pattern here as I mentioned before. Move Symbol uniqueSymb outside the #ifdef ... #else ... #endif block and place the common code after the block so you don't have to write it twice.

@JckXia
Copy link
Member Author

JckXia commented Nov 14, 2021

@mhdawson @gabrielschulhof @KevinEady @NickNaso Hello everyone. I updated this pull request to opt using hard-coded strings as the property key on the error object, due to issues that came with using Symbol::For. Thanks!

@mhdawson
Copy link
Member

@mhdawson @gabrielschulhof @KevinEady @NickNaso do you want to take a look before I land this, if so let me know before the end of the week. If I don't get an ask for more time to review I'll go ahead and land on Friday.

@mhdawson
Copy link
Member

@JckXia I see one comment from @gabrielschulhof that I don't think you addressed the one about lines 2620 and 2621

@JckXia
Copy link
Member Author

JckXia commented Nov 16, 2021

@mhdawson I believe Gabriel's comment originally was addressing the fact that I was using a separate flag to denote the error object is wrapped on top of having an errorVal property. I removed one of the property descriptors and now we only attach one property 4bda9e7e-4913-4dbc-95de-891cbf66598e-errorVal to show that the err obj is wrapped

e0567d0

@mhdawson
Copy link
Member

@JckXia thanks for the clarification.

@JckXia
Copy link
Member Author

JckXia commented Nov 22, 2021

Hey guys @mhdawson @gabrielschulhof @NickNaso @KevinEady. Could you take a look at this PR again? Thanks!

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM, looks good to me with all the update. @JckXia thanks for your work on this.

Copy link
Contributor

@KevinEady KevinEady left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for clarifying the need for the string key in today's Node.JS API meeting.

@JckXia JckXia merged commit 293c732 into nodejs:main Nov 26, 2021
@JckXia
Copy link
Member Author

JckXia commented Nov 26, 2021

Landed as 293c732

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.

6 participants