n-api: Enable scope and ref APIs during exception #12524
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
c++
|
@mhdawson @gabrielschulhof please review. |
src/node_api.cc
Outdated
| CHECK_ARG(env, result); | ||
|
|
||
| v8impl::Reference* reference = v8impl::Reference::New( | ||
| env, v8impl::V8LocalValueFromJsValue(value), initial_refcount, false); | ||
|
|
||
| *result = reinterpret_cast<napi_ref>(reference); | ||
| return GET_RETURN_STATUS(env); | ||
| return napi_ok; |
There was a problem hiding this comment.
NAPI_PREAMBLE(env) clears the last error, whereas CHECK_ENV(env) does not. So, the public APIs which do not start with NAPI_PREAMBLE(env) leave the last error status as it was if they simply return napi_ok. Thus, I was thinking we should change napi_clear_last_error(env) to always return napi_ok and then change all these return napi_ok; statements to return napi_clear_last_error(env);
gabrielschulhof
approved these changes
Apr 20, 2017
|
... and also LGTM because |
|
This will need to be merged with #12539. It might be better to let that one go first, then I can rebase and update this PR to use the new patterns from that one. |
|
Ok, I'll wait and land the other one first. |
2 tasks
N-API is somewhat strict about blocking calls to many APIs while there is a pending exception. The NAPI_PREAMBLE macro at the beginning of many API implementations checks for a pending exception. However, a subset of the APIs (which don't call back into JavaScript) still need to work while in a pending-exception state. This changes the reference APIs (equivalent to v8::Persistent) and handle scope APIs so that they can be used for cleanup up while an exception is pending. We may decide to similarly enable a few other APIs later, (which would be a non-breaking change) but we know at least these are needed now to unblock some specific scenarios. Fixes: nodejs/abi-stable-node#122
jasongin
force-pushed
the
pending_exception
branch
from
a8c4e43 to
9956faa
Compare
|
Rebased, and added a commit to conform to the updated pattern in #12539 where we return |
addaleax
approved these changes
Apr 25, 2017
|
Oh, this also fixes nodejs/abi-stable-node#228 |
|
Landed in b7a341d and added the extra Fixes: tag, thanks! |
addaleax
pushed a commit
that referenced
this pull request
Apr 25, 2017
N-API is somewhat strict about blocking calls to many APIs while there is a pending exception. The NAPI_PREAMBLE macro at the beginning of many API implementations checks for a pending exception. However, a subset of the APIs (which don't call back into JavaScript) still need to work while in a pending-exception state. This changes the reference APIs (equivalent to v8::Persistent) and handle scope APIs so that they can be used for cleanup up while an exception is pending. We may decide to similarly enable a few other APIs later, (which would be a non-breaking change) but we know at least these are needed now to unblock some specific scenarios. Fixes: nodejs/abi-stable-node#122 Fixes: nodejs/abi-stable-node#228 PR-URL: #12524 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
gabrielschulhof
pushed a commit
to gabrielschulhof/node
that referenced
this pull request
Apr 10, 2018
N-API is somewhat strict about blocking calls to many APIs while there is a pending exception. The NAPI_PREAMBLE macro at the beginning of many API implementations checks for a pending exception. However, a subset of the APIs (which don't call back into JavaScript) still need to work while in a pending-exception state. This changes the reference APIs (equivalent to v8::Persistent) and handle scope APIs so that they can be used for cleanup up while an exception is pending. We may decide to similarly enable a few other APIs later, (which would be a non-breaking change) but we know at least these are needed now to unblock some specific scenarios. Fixes: nodejs/abi-stable-node#122 Fixes: nodejs/abi-stable-node#228 PR-URL: nodejs#12524 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
MylesBorins
pushed a commit
that referenced
this pull request
Apr 16, 2018
N-API is somewhat strict about blocking calls to many APIs while there is a pending exception. The NAPI_PREAMBLE macro at the beginning of many API implementations checks for a pending exception. However, a subset of the APIs (which don't call back into JavaScript) still need to work while in a pending-exception state. This changes the reference APIs (equivalent to v8::Persistent) and handle scope APIs so that they can be used for cleanup up while an exception is pending. We may decide to similarly enable a few other APIs later, (which would be a non-breaking change) but we know at least these are needed now to unblock some specific scenarios. Fixes: nodejs/abi-stable-node#122 Fixes: nodejs/abi-stable-node#228 Backport-PR-URL: #19447 PR-URL: #12524 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
N-API is somewhat strict about blocking calls to many APIs while there is a pending exception. The
NAPI_PREAMBLEmacro at the beginning of many API implementations checks for a pending exception. However, a subset of the APIs (which don't call back into JavaScript) still need to work while in a pending-exception state. This changes the reference APIs (equivalent tov8::Persistent) and handle scope APIs so that they can be used for cleanup up while an exception is pending.We may decide to similarly enable a few other APIs later, (which would be a non-breaking change) but we know at least these are needed now to unblock some specific scenarios.
There currently isn't good test coverage of the reference APIs; I'm working on that separately. This change only adds a handle scope test case.
Also fixing a couple places where I noticed the argument validation was incorrect.
Fixes: nodejs/abi-stable-node#122
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)