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

src: handle failure during error wrap of primitive #1310

Conversation

gabrielschulhof
Copy link
Contributor

When we wrap a primitive value into an object in order to throw it as an error, we call napi_define_properties() which may return napi_pending_exception if the environment is shutting down. Handle this case when NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS is given by checking whether we're in an environment shutdown scenario and ignore the failure of napi_define_properties(), since the error will not reach JS anyway.

When we wrap a primitive value into an object in order to throw it as
an error, we call `napi_define_properties()` which may return
`napi_pending_exception` if the environment is shutting down. Handle
this case when `NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS` is given by
checking whether we're in an environment shutdown scenario and ignore
the failure of `napi_define_properties()`, since the error will not
reach JS anyway.
@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Apr 14, 2023

Note that this change will become mostly dead code because we're only down this path if napi_create_reference() for the value that is to become our error returns non-napi_ok. Once nodejs/node#45715 lands, and for newer Node-API, napi_create_reference() should almost always return napi_ok, no matter what the value.

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

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Apr 29, 2023

gabrielschulhof added a commit that referenced this pull request Apr 29, 2023
When we wrap a primitive value into an object in order to throw it as
an error, we call `napi_define_properties()` which may return
`napi_pending_exception` if the environment is shutting down. Handle
this case when `NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS` is given by
checking whether we're in an environment shutdown scenario and ignore
the failure of `napi_define_properties()`, since the error will not
reach JS anyway.

Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
PR-URL: #1310
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@gabrielschulhof
Copy link
Contributor Author

Landed in 64f6515.

@gabrielschulhof gabrielschulhof deleted the fail-gracefully-on-generic-exception branch April 29, 2023 03:35
austinli64 added a commit to austinli64/node-addon-api that referenced this pull request May 9, 2023
When we wrap a primitive value into an object in order to throw it as
an error, we call `napi_define_properties()` which may return
`napi_pending_exception` if the environment is shutting down. Handle
this case when `NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS` is given by
checking whether we're in an environment shutdown scenario and ignore
the failure of `napi_define_properties()`, since the error will not
reach JS anyway.

Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
PR-URL: nodejs/node-addon-api#1310
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
When we wrap a primitive value into an object in order to throw it as
an error, we call `napi_define_properties()` which may return
`napi_pending_exception` if the environment is shutting down. Handle
this case when `NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS` is given by
checking whether we're in an environment shutdown scenario and ignore
the failure of `napi_define_properties()`, since the error will not
reach JS anyway.

Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
PR-URL: nodejs/node-addon-api#1310
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.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 this pull request may close these issues.

None yet

3 participants