Skip to content

Commit b7a341d

Browse files
jasonginaddaleax
authored andcommitted
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 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]>
1 parent 42dca99 commit b7a341d

File tree

3 files changed

+72
-20
lines changed

3 files changed

+72
-20
lines changed

src/node_api.cc

+40-20
Original file line numberDiff line numberDiff line change
@@ -2043,15 +2043,17 @@ napi_status napi_create_reference(napi_env env,
20432043
napi_value value,
20442044
uint32_t initial_refcount,
20452045
napi_ref* result) {
2046-
NAPI_PREAMBLE(env);
2046+
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
2047+
// JS exceptions.
2048+
CHECK_ENV(env);
20472049
CHECK_ARG(env, value);
20482050
CHECK_ARG(env, result);
20492051

20502052
v8impl::Reference* reference = v8impl::Reference::New(
20512053
env, v8impl::V8LocalValueFromJsValue(value), initial_refcount, false);
20522054

20532055
*result = reinterpret_cast<napi_ref>(reference);
2054-
return GET_RETURN_STATUS(env);
2056+
return napi_clear_last_error(env);
20552057
}
20562058

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

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

2086-
return GET_RETURN_STATUS(env);
2090+
return napi_clear_last_error(env);
20872091
}
20882092

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

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

2109-
return GET_RETURN_STATUS(env);
2115+
return napi_clear_last_error(env);
21102116
}
21112117

21122118
// Attempts to get a referenced value. If the reference is weak, the value might
@@ -2115,59 +2121,71 @@ napi_status napi_reference_unref(napi_env env, napi_ref ref, uint32_t* result) {
21152121
napi_status napi_get_reference_value(napi_env env,
21162122
napi_ref ref,
21172123
napi_value* result) {
2118-
NAPI_PREAMBLE(env);
2124+
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
2125+
// JS exceptions.
2126+
CHECK_ENV(env);
21192127
CHECK_ARG(env, ref);
21202128
CHECK_ARG(env, result);
21212129

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

2125-
return GET_RETURN_STATUS(env);
2133+
return napi_clear_last_error(env);
21262134
}
21272135

21282136
napi_status napi_open_handle_scope(napi_env env, napi_handle_scope* result) {
2129-
NAPI_PREAMBLE(env);
2137+
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
2138+
// JS exceptions.
2139+
CHECK_ENV(env);
21302140
CHECK_ARG(env, result);
21312141

21322142
*result = v8impl::JsHandleScopeFromV8HandleScope(
21332143
new v8impl::HandleScopeWrapper(env->isolate));
2134-
return GET_RETURN_STATUS(env);
2144+
return napi_clear_last_error(env);
21352145
}
21362146

21372147
napi_status napi_close_handle_scope(napi_env env, napi_handle_scope scope) {
2138-
NAPI_PREAMBLE(env);
2148+
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
2149+
// JS exceptions.
2150+
CHECK_ENV(env);
21392151
CHECK_ARG(env, scope);
21402152

21412153
delete v8impl::V8HandleScopeFromJsHandleScope(scope);
2142-
return GET_RETURN_STATUS(env);
2154+
return napi_clear_last_error(env);
21432155
}
21442156

21452157
napi_status napi_open_escapable_handle_scope(
21462158
napi_env env,
21472159
napi_escapable_handle_scope* result) {
2148-
NAPI_PREAMBLE(env);
2160+
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
2161+
// JS exceptions.
2162+
CHECK_ENV(env);
21492163
CHECK_ARG(env, result);
21502164

21512165
*result = v8impl::JsEscapableHandleScopeFromV8EscapableHandleScope(
21522166
new v8impl::EscapableHandleScopeWrapper(env->isolate));
2153-
return GET_RETURN_STATUS(env);
2167+
return napi_clear_last_error(env);
21542168
}
21552169

21562170
napi_status napi_close_escapable_handle_scope(
21572171
napi_env env,
21582172
napi_escapable_handle_scope scope) {
2159-
NAPI_PREAMBLE(env);
2173+
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
2174+
// JS exceptions.
2175+
CHECK_ENV(env);
21602176
CHECK_ARG(env, scope);
21612177

21622178
delete v8impl::V8EscapableHandleScopeFromJsEscapableHandleScope(scope);
2163-
return GET_RETURN_STATUS(env);
2179+
return napi_clear_last_error(env);
21642180
}
21652181

21662182
napi_status napi_escape_handle(napi_env env,
21672183
napi_escapable_handle_scope scope,
21682184
napi_value escapee,
21692185
napi_value* result) {
2170-
NAPI_PREAMBLE(env);
2186+
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
2187+
// JS exceptions.
2188+
CHECK_ENV(env);
21712189
CHECK_ARG(env, scope);
21722190
CHECK_ARG(env, escapee);
21732191
CHECK_ARG(env, result);
@@ -2176,7 +2194,7 @@ napi_status napi_escape_handle(napi_env env,
21762194
v8impl::V8EscapableHandleScopeFromJsEscapableHandleScope(scope);
21772195
*result = v8impl::JsValueFromV8LocalValue(
21782196
s->Escape(v8impl::V8LocalValueFromJsValue(escapee)));
2179-
return GET_RETURN_STATUS(env);
2197+
return napi_clear_last_error(env);
21802198
}
21812199

21822200
napi_status napi_new_instance(napi_env env,
@@ -2386,7 +2404,6 @@ napi_status napi_create_buffer(napi_env env,
23862404
void** data,
23872405
napi_value* result) {
23882406
NAPI_PREAMBLE(env);
2389-
CHECK_ARG(env, data);
23902407
CHECK_ARG(env, result);
23912408

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

23982415
*result = v8impl::JsValueFromV8LocalValue(buffer);
2399-
*data = node::Buffer::Data(buffer);
2416+
2417+
if (data != nullptr) {
2418+
*data = node::Buffer::Data(buffer);
2419+
}
24002420

24012421
return GET_RETURN_STATUS(env);
24022422
}

test/addons-napi/test_handle_scope/test.js

+7
Original file line numberDiff line numberDiff line change
@@ -7,4 +7,11 @@ const testHandleScope =
77
require(`./build/${common.buildType}/test_handle_scope`);
88

99
testHandleScope.NewScope();
10+
1011
assert.ok(testHandleScope.NewScopeEscape() instanceof Object);
12+
13+
assert.throws(
14+
() => {
15+
testHandleScope.NewScopeWithException(() => { throw new RangeError(); });
16+
},
17+
RangeError);

test/addons-napi/test_handle_scope/test_handle_scope.c

+25
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,35 @@ napi_value NewScopeEscape(napi_env env, napi_callback_info info) {
2929
return escapee;
3030
}
3131

32+
napi_value NewScopeWithException(napi_env env, napi_callback_info info) {
33+
napi_handle_scope scope;
34+
size_t argc;
35+
napi_value exception_function;
36+
napi_status status;
37+
napi_value output = NULL;
38+
39+
NAPI_CALL(env, napi_open_handle_scope(env, &scope));
40+
NAPI_CALL(env, napi_create_object(env, &output));
41+
42+
argc = 1;
43+
NAPI_CALL(env, napi_get_cb_info(
44+
env, info, &argc, &exception_function, NULL, NULL));
45+
46+
status = napi_call_function(
47+
env, output, exception_function, 0, NULL, NULL);
48+
NAPI_ASSERT(env, status == napi_pending_exception,
49+
"Function should have thrown.");
50+
51+
// Closing a handle scope should still work while an exception is pending.
52+
NAPI_CALL(env, napi_close_handle_scope(env, scope));
53+
return NULL;
54+
}
55+
3256
void Init(napi_env env, napi_value exports, napi_value module, void* priv) {
3357
napi_property_descriptor properties[] = {
3458
DECLARE_NAPI_PROPERTY("NewScope", NewScope),
3559
DECLARE_NAPI_PROPERTY("NewScopeEscape", NewScopeEscape),
60+
DECLARE_NAPI_PROPERTY("NewScopeWithException", NewScopeWithException),
3661
};
3762

3863
NAPI_CALL_RETURN_VOID(env, napi_define_properties(

0 commit comments

Comments
 (0)