Fix reporting of API error messages#43
Conversation
When a call to an N-API function caused an error for some reason other than a JS exception, the fallback error message "Error in native callback" was always reported because of incorrect logic in `Napi::Error::New()`. Then that fix exposed a bug in `napi_get_last_error_info()`, which I have fixed here and also at nodejs/node#13087
| status = napi_get_last_error_info(env, &info); | ||
| assert(status == napi_ok); | ||
|
|
||
| if (status == napi_ok) { |
There was a problem hiding this comment.
Does this make sense? assert + then make the code conditional on asserted statement?
There was a problem hiding this comment.
If we encounter another error while handling an error, there's not much we can do. Probably it should be a fatal error, except we haven't exposed node::FatalError() via N-API yet. The assert will at least indicate when something went wrong in a debug build, but in a release build the conditional allows it to continue without crashing.
There was a problem hiding this comment.
This came up last time this code was reviewed: #32 (comment)
Probably we should go ahead and add a napi_fatal_error() API.
There was a problem hiding this comment.
I opened a new issue to track this: #48
I don't think the current PR needs to be blocked by it.
|
@addaleax @boingoing @mhdawson Can someone review this? Thanks! |
boingoing
left a comment
boingoing
left a comment
There was a problem hiding this comment.
Looks good to me. Agree with other comments if napi_get_last_error_info returns an error we should fatal error.
3 participants
When a call to an N-API function caused an error for some reason other than a JS exception, the fallback error message "Error in native callback" was always reported because of incorrect logic in
Napi::Error::New().Then that fix exposed a bug in
napi_get_last_error_info(), which I have fixed here and also at nodejs/node#13087