Skip to content

Commit

Permalink
n-api: Enable scope and ref APIs during exception
Browse files Browse the repository at this point in the history 
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]>
  • Loading branch information
@jasongin @addaleax
jasongin authored and addaleax committed Apr 25, 2017
1 parent 42dca99 commit b7a341d
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 20 deletions.
60 changes: 40 additions & 20 deletions src/node_api.cc
View file
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_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_clear_last_error(env);
}

// 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_clear_last_error(env);
}

// 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_clear_last_error(env);
}

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_clear_last_error(env);
}

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_clear_last_error(env);
}

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_clear_last_error(env);
}

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_clear_last_error(env);
}

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_clear_last_error(env);
}

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

0 comments on commit b7a341d

Please sign in to comment.