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

Question about how Error::New() handles last error and exceptions #1089

Closed
JckXia opened this issue Oct 9, 2021 · 8 comments
Closed

Question about how Error::New() handles last error and exceptions #1089

JckXia opened this issue Oct 9, 2021 · 8 comments

Comments

@JckXia
Copy link
Member

JckXia commented Oct 9, 2021

I was writing a small Cpp addon trying to see if there exists a scenario where the _ref class variable of Error class might be a nullptr.
TestAddon
Since it looks like the error_code napi_boolean_expected isn't handled by the switch statement inside Error::New().

Though it looks like even though we are able to retrieve the error info using napi_get_last_error_info , once we call napi_is_exception_pending we will clear the last error and thus lose the error info from the previous invocation.
newErrorCode

I am just wondering if this is the expected behaviour? Since it looks like the original error info( a const) is intended to be used elsewhere in the function body. Thanks!

@KevinEady
Copy link
Contributor

Whenever I see a napi_* without a napi_status status = or NAPI_THROW_ in front of it, it triggers a red flag for me. You should check the return status of your napi_get_value_bool immediately after invocation. We provide the NAPI_THROW_* macros for that. And when the APIs are used incorrectly, I am not sure the intended behavior. Eg you immediately create an Error object not really knowing if one should be made (which is handled by the NAPI_THROW_* macros).

What happens if you swap the order of the get_last_error_info and napi_is_exception_pending calls?

@JckXia
Copy link
Member Author

JckXia commented Oct 11, 2021

Hmm, I think in this instance we know that napi_get_value_bool will register an error upon invocation since "asd" is not a bool value, and NAPI_THROW_* looks like it will construct an Error::New(env) value regardless (with the same behaviour as aforementioned, the initial error code being overwritten with a subseq call to napi_is_exception_pending).

When I swap the get_last_error_info and napi_is_exception_pending, the error code napi_boolean_expected gets overwritten completely. I think this is because inside napi_is_exception_pending, we call napi_clear_last_error(env) which simply updates the error code to napi_ok.

It also looks like even though the error code gets overwritten, the error message persists because napi_clear_last_error does not mutate that field. So we have a situation where the error code is napi_ok but the error message is inconsistent with the error code.

@mhdawson
Copy link
Member

mhdawson commented Oct 13, 2021

This section describes the planned error handling: https://nodejs.org/api/n-api.html#n_api_return_values

The TLDR; is that the doc covers

  1. Always check the status returned from a call immediately after it is called
  2. If the result is not napi_ok or napi_pending_exception you must also call napi_pending_exception to see if there is a pending exception.
  3. napi_get_last_error_info can be called to get additional info on a status code. What it does not state specifically is that you need to call this immediately after making the original call that returned something other than napi_ok. We could add that. @JckXia do you think that would help?

Separately, we might also want to look at napi_get_last_error_info, and see if we should reset the napi_extened_error_info returned. We could quite likely defer that until napi_get_last_error_info is called (ie clear if the last error is now napi_ok) which would mean we would not add overhead unless napi_get_last_error_info is called when the error has been reset.

@JckXia
Copy link
Member Author

JckXia commented Oct 13, 2021

Hey @mhdawson , yeah I think adding that to the doc could help. Though the issue with the Error::New(env) implementation could that the napi_is_exception_pending call mutates the napi_extended_error_info struct, while the switch statement in later parts of the code seems to work with the assumption that info->error_code has not been changed . In this case, should we create a copy of the info struct?

ErrImpl

@mhdawson
Copy link
Member

Yes, I see that the code code does not expect that the data pointed to by info be changed by later calls. We re-use the same structure to avoid allocating/de-allocating but does result in this issue.

As you suggest the method should pull the fields it uses out of info earlier to avoid that.

mhdawson added a commit to mhdawson/io.js that referenced this issue Oct 14, 2021
Fix up example and make it more explicit on how
you need to use napi_extended_error_info in order to
help people avoid what might be a common mistake that
we made in node-addon-api.

Refs: nodejs/node-addon-api#1089

Signed-off-by: Michael Dawson <mdawson@devrus.com>
@mhdawson
Copy link
Member

I created this PR to address the doc side: nodejs/node#40458

@JckXia do you want to submit a PR to clear info in napi_get_last_error_info() if the last status was napi_ok?

RaisinTen added a commit to RaisinTen/node-addon-api that referenced this issue Oct 17, 2021
We must create a copy of the last error info before using it if
subsequent Node-API function calls are involved as those may mutate the
fields of the original structure.

Fixes: nodejs#1089
Signed-off-by: Darshan Sen <darshan.sen@postman.com>
RaisinTen added a commit to RaisinTen/node-addon-api that referenced this issue Oct 17, 2021
All fields of the `napi_extended_error_info` structure, except
`error_message`, gets reset in subsequent Node-API function calls on the
same `env`. This includes a call to `napi_is_exception_pending()`. So
here it is necessary to make a copy of the information as the
`error_code` field is used later on.

Fixes: nodejs#1089
Signed-off-by: Darshan Sen <darshan.sen@postman.com>
@RaisinTen
Copy link
Contributor

PR: #1092

RaisinTen added a commit to RaisinTen/node-addon-api that referenced this issue Oct 20, 2021
All fields of the `napi_extended_error_info` structure gets reset in
subsequent Node-API function calls on the same `env`. This includes a
call to `napi_is_exception_pending()`. So here it is necessary to make
a copy of the information as the `error_code` field is used later on.

Fixes: nodejs#1089
Signed-off-by: Darshan Sen <darshan.sen@postman.com>
mhdawson added a commit to nodejs/node that referenced this issue Oct 20, 2021
Fix up example and make it more explicit on how
you need to use napi_extended_error_info in order to
help people avoid what might be a common mistake that
we made in node-addon-api.

Refs: nodejs/node-addon-api#1089

Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #40458
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@JckXia
Copy link
Member Author

JckXia commented Oct 21, 2021

Currently working on a PR to reset the error info if previous status is napi_ok inside node core

targos pushed a commit to nodejs/node that referenced this issue Oct 23, 2021
Fix up example and make it more explicit on how
you need to use napi_extended_error_info in order to
help people avoid what might be a common mistake that
we made in node-addon-api.

Refs: nodejs/node-addon-api#1089

Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #40458
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
mhdawson pushed a commit to nodejs/node that referenced this issue Nov 17, 2021
PR-URL: #40552
Refs: nodejs/node-addon-api#1089
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
targos pushed a commit to nodejs/node that referenced this issue Nov 21, 2021
PR-URL: #40552
Refs: nodejs/node-addon-api#1089
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
BethGriggs pushed a commit to nodejs/node that referenced this issue Nov 23, 2021
Fix up example and make it more explicit on how
you need to use napi_extended_error_info in order to
help people avoid what might be a common mistake that
we made in node-addon-api.

Refs: nodejs/node-addon-api#1089

Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #40458
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
danielleadams pushed a commit to nodejs/node that referenced this issue Jan 30, 2022
PR-URL: #40552
Refs: nodejs/node-addon-api#1089
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
danielleadams pushed a commit to nodejs/node that referenced this issue Feb 1, 2022
PR-URL: #40552
Refs: nodejs/node-addon-api#1089
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this issue Aug 24, 2022
All fields of the `napi_extended_error_info` structure gets reset in
subsequent Node-API function calls on the same `env`. This includes a
call to `napi_is_exception_pending()`. So here it is necessary to make
a copy of the information as the `error_code` field is used later on.

Signed-off-by: Darshan Sen <darshan.sen@postman.com>

PR-URL: nodejs/node-addon-api#1092
Fixes: nodejs/node-addon-api#1089
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this issue Aug 26, 2022
All fields of the `napi_extended_error_info` structure gets reset in
subsequent Node-API function calls on the same `env`. This includes a
call to `napi_is_exception_pending()`. So here it is necessary to make
a copy of the information as the `error_code` field is used later on.

Signed-off-by: Darshan Sen <darshan.sen@postman.com>

PR-URL: nodejs/node-addon-api#1092
Fixes: nodejs/node-addon-api#1089
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this issue Sep 19, 2022
All fields of the `napi_extended_error_info` structure gets reset in
subsequent Node-API function calls on the same `env`. This includes a
call to `napi_is_exception_pending()`. So here it is necessary to make
a copy of the information as the `error_code` field is used later on.

Signed-off-by: Darshan Sen <darshan.sen@postman.com>

PR-URL: nodejs/node-addon-api#1092
Fixes: nodejs/node-addon-api#1089
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this issue Aug 11, 2023
All fields of the `napi_extended_error_info` structure gets reset in
subsequent Node-API function calls on the same `env`. This includes a
call to `napi_is_exception_pending()`. So here it is necessary to make
a copy of the information as the `error_code` field is used later on.

Signed-off-by: Darshan Sen <darshan.sen@postman.com>

PR-URL: nodejs/node-addon-api#1092
Fixes: nodejs/node-addon-api#1089
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
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 a pull request may close this issue.

4 participants