Skip to content

n-api: Enable scope and ref APIs during exception #12524

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.

Sign up for GitHub

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

Closed
jasongin wants to merge 2 commits into from
Closed

n-api: Enable scope and ref APIs during exception #12524

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
18fad96
n-api: Enable scope and ref APIs during exception
jasongin Apr 19, 2017
9956faa
Return napi_clear_last_error() where NAPI_PREAMBLE was removed
jasongin Apr 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
n-api: Enable scope and ref APIs during exception 
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
  • Loading branch information
@jasongin
jasongin committed Apr 25, 2017
commit 18fad96924afa3f2c34d0a590a015cec85a06856
60 changes: 40 additions & 20 deletions src/node_api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2043,15 +2043,17 @@ napi_status napi_create_reference(napi_env env,
napi_value value,
uint32_t initial_refcount,
napi_ref* result) {
NAPI_PREAMBLE(env);
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
// JS exceptions.
CHECK_ENV(env);
CHECK_ARG(env, value);
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;
Copy link
Copy Markdown
Contributor

@gabrielschulhof gabrielschulhof Apr 20, 2017

Choose a reason for hiding this comment

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

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);

}

// Deletes a reference. The referenced value is released, and may be GC'd unless
Expand All @@ -2073,7 +2075,9 @@ napi_status napi_delete_reference(napi_env env, napi_ref ref) {
// Calling this when the refcount is 0 and the object is unavailable
// results in an error.
napi_status napi_reference_ref(napi_env env, napi_ref ref, uint32_t* result) {
NAPI_PREAMBLE(env);
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
// JS exceptions.
CHECK_ENV(env);
CHECK_ARG(env, ref);

v8impl::Reference* reference = reinterpret_cast<v8impl::Reference*>(ref);
Expand All @@ -2083,15 +2087,17 @@ napi_status napi_reference_ref(napi_env env, napi_ref ref, uint32_t* result) {
*result = count;
}

return GET_RETURN_STATUS(env);
return napi_ok;
}

// Decrements the reference count, optionally returning the resulting count. If
// the result is 0 the reference is now weak and the object may be GC'd at any
// time if there are no other references. Calling this when the refcount is
// already 0 results in an error.
napi_status napi_reference_unref(napi_env env, napi_ref ref, uint32_t* result) {
NAPI_PREAMBLE(env);
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
// JS exceptions.
CHECK_ENV(env);
CHECK_ARG(env, ref);

v8impl::Reference* reference = reinterpret_cast<v8impl::Reference*>(ref);
Expand All @@ -2106,7 +2112,7 @@ napi_status napi_reference_unref(napi_env env, napi_ref ref, uint32_t* result) {
*result = count;
}

return GET_RETURN_STATUS(env);
return napi_ok;
}

// Attempts to get a referenced value. If the reference is weak, the value might
Expand All @@ -2115,59 +2121,71 @@ napi_status napi_reference_unref(napi_env env, napi_ref ref, uint32_t* result) {
napi_status napi_get_reference_value(napi_env env,
napi_ref ref,
napi_value* result) {
NAPI_PREAMBLE(env);
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
// JS exceptions.
CHECK_ENV(env);
CHECK_ARG(env, ref);
CHECK_ARG(env, result);

v8impl::Reference* reference = reinterpret_cast<v8impl::Reference*>(ref);
*result = v8impl::JsValueFromV8LocalValue(reference->Get());

return GET_RETURN_STATUS(env);
return napi_ok;
}

napi_status napi_open_handle_scope(napi_env env, napi_handle_scope* result) {
NAPI_PREAMBLE(env);
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
// JS exceptions.
CHECK_ENV(env);
CHECK_ARG(env, result);

*result = v8impl::JsHandleScopeFromV8HandleScope(
new v8impl::HandleScopeWrapper(env->isolate));
return GET_RETURN_STATUS(env);
return napi_ok;
}

napi_status napi_close_handle_scope(napi_env env, napi_handle_scope scope) {
NAPI_PREAMBLE(env);
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
// JS exceptions.
CHECK_ENV(env);
CHECK_ARG(env, scope);

delete v8impl::V8HandleScopeFromJsHandleScope(scope);
return GET_RETURN_STATUS(env);
return napi_ok;
}

napi_status napi_open_escapable_handle_scope(
napi_env env,
napi_escapable_handle_scope* result) {
NAPI_PREAMBLE(env);
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
// JS exceptions.
CHECK_ENV(env);
CHECK_ARG(env, result);

*result = v8impl::JsEscapableHandleScopeFromV8EscapableHandleScope(
new v8impl::EscapableHandleScopeWrapper(env->isolate));
return GET_RETURN_STATUS(env);
return napi_ok;
}

napi_status napi_close_escapable_handle_scope(
napi_env env,
napi_escapable_handle_scope scope) {
NAPI_PREAMBLE(env);
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
// JS exceptions.
CHECK_ENV(env);
CHECK_ARG(env, scope);

delete v8impl::V8EscapableHandleScopeFromJsEscapableHandleScope(scope);
return GET_RETURN_STATUS(env);
return napi_ok;
}

napi_status napi_escape_handle(napi_env env,
napi_escapable_handle_scope scope,
napi_value escapee,
napi_value* result) {
NAPI_PREAMBLE(env);
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
// JS exceptions.
CHECK_ENV(env);
CHECK_ARG(env, scope);
CHECK_ARG(env, escapee);
CHECK_ARG(env, result);
Expand All @@ -2176,7 +2194,7 @@ napi_status napi_escape_handle(napi_env env,
v8impl::V8EscapableHandleScopeFromJsEscapableHandleScope(scope);
*result = v8impl::JsValueFromV8LocalValue(
s->Escape(v8impl::V8LocalValueFromJsValue(escapee)));
return GET_RETURN_STATUS(env);
return napi_ok;
}

napi_status napi_new_instance(napi_env env,
Expand Down Expand Up @@ -2386,7 +2404,6 @@ napi_status napi_create_buffer(napi_env env,
void** data,
napi_value* result) {
NAPI_PREAMBLE(env);
CHECK_ARG(env, data);
CHECK_ARG(env, result);

auto maybe = node::Buffer::New(env->isolate, length);
Expand All @@ -2396,7 +2413,10 @@ napi_status napi_create_buffer(napi_env env,
v8::Local<v8::Object> buffer = maybe.ToLocalChecked();

*result = v8impl::JsValueFromV8LocalValue(buffer);
*data = node::Buffer::Data(buffer);

if (data != nullptr) {
*data = node::Buffer::Data(buffer);
}

return GET_RETURN_STATUS(env);
}
Expand Down
7 changes: 7 additions & 0 deletions test/addons-napi/test_handle_scope/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,11 @@ const testHandleScope =
require(`./build/${common.buildType}/test_handle_scope`);

testHandleScope.NewScope();

assert.ok(testHandleScope.NewScopeEscape() instanceof Object);

assert.throws(
() => {
testHandleScope.NewScopeWithException(() => { throw new RangeError(); });
},
RangeError);
25 changes: 25 additions & 0 deletions test/addons-napi/test_handle_scope/test_handle_scope.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,35 @@ napi_value NewScopeEscape(napi_env env, napi_callback_info info) {
return escapee;
}

napi_value NewScopeWithException(napi_env env, napi_callback_info info) {
napi_handle_scope scope;
size_t argc;
napi_value exception_function;
napi_status status;
napi_value output = NULL;

NAPI_CALL(env, napi_open_handle_scope(env, &scope));
NAPI_CALL(env, napi_create_object(env, &output));

argc = 1;
NAPI_CALL(env, napi_get_cb_info(
env, info, &argc, &exception_function, NULL, NULL));

status = napi_call_function(
env, output, exception_function, 0, NULL, NULL);
NAPI_ASSERT(env, status == napi_pending_exception,
"Function should have thrown.");

// Closing a handle scope should still work while an exception is pending.
NAPI_CALL(env, napi_close_handle_scope(env, scope));
return NULL;
}

void Init(napi_env env, napi_value exports, napi_value module, void* priv) {
napi_property_descriptor properties[] = {
DECLARE_NAPI_PROPERTY("NewScope", NewScope),
DECLARE_NAPI_PROPERTY("NewScopeEscape", NewScopeEscape),
DECLARE_NAPI_PROPERTY("NewScopeWithException", NewScopeWithException),
};

NAPI_CALL_RETURN_VOID(env, napi_define_properties(
Expand Down