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

n-api: add methods to open/close callback scope #18089

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
38 changes: 38 additions & 0 deletions doc/api/n-api.md
Original file line number Diff line number Diff line change
Expand Up @@ -3434,6 +3434,42 @@ is sufficient and appropriate. Use of the `napi_make_callback` function
may be required when implementing custom async behavior that does not use
`napi_create_async_work`.

### *napi_open_callback_scope*
<!-- YAML
added: REPLACEME
-->
```C
NAPI_EXTERN napi_status napi_open_callback_scope(napi_env env,
napi_value resource_object,
napi_async_context context,
napi_callback_scope* result)
```
- `[in] env`: The environment that the API is invoked under.
- `[in] resource_object`: An optional object associated with the async work
that will be passed to possible async_hooks [`init` hooks][].
- `[in] context`: Context for the async operation that is
invoking the callback. This should be a value previously obtained
from [`napi_async_init`][].
- `[out] result`: The newly created scope.

There are cases(for example resolving promises) where it is
necessary to have the equivalent of the scope associated with a callback
in place when making certain N-API calls. If there is no other script on
the stack the [`napi_open_callback_scope`][] and
[`napi_close_callback_scope`][] functions can be used to open/close
the required scope.

### *napi_close_callback_scope*
<!-- YAML
added: REPLACEME
-->
```C
NAPI_EXTERN napi_status napi_close_callback_scope(napi_env env,
napi_callback_scope scope)
```
- `[in] env`: The environment that the API is invoked under.
- `[in] scope`: The scope to be closed.

## Version Management

### napi_get_node_version
Expand Down Expand Up @@ -3719,6 +3755,7 @@ NAPI_EXTERN napi_status napi_get_uv_event_loop(napi_env env,
[`napi_async_init`]: #n_api_napi_async_init
[`napi_cancel_async_work`]: #n_api_napi_cancel_async_work
[`napi_close_escapable_handle_scope`]: #n_api_napi_close_escapable_handle_scope
[`napi_close_callback_scope`]: #n_api_napi_close_callback_scope
[`napi_close_handle_scope`]: #n_api_napi_close_handle_scope
[`napi_create_async_work`]: #n_api_napi_create_async_work
[`napi_create_error`]: #n_api_napi_create_error
Expand All @@ -3744,6 +3781,7 @@ NAPI_EXTERN napi_status napi_get_uv_event_loop(napi_env env,
[`napi_get_last_error_info`]: #n_api_napi_get_last_error_info
[`napi_get_and_clear_last_exception`]: #n_api_napi_get_and_clear_last_exception
[`napi_make_callback`]: #n_api_napi_make_callback
[`napi_open_callback_scope`]: #n_api_napi_open_callback_scope
[`napi_open_escapable_handle_scope`]: #n_api_napi_open_escapable_handle_scope
[`napi_open_handle_scope`]: #n_api_napi_open_handle_scope
[`napi_property_descriptor`]: #n_api_napi_property_descriptor
Expand Down
62 changes: 59 additions & 3 deletions src/node_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ struct napi_env__ {
v8::Persistent<v8::ObjectTemplate> accessor_data_template;
napi_extended_error_info last_error;
int open_handle_scopes = 0;
int open_callback_scopes = 0;
uv_loop_t* loop = nullptr;
};

Expand Down Expand Up @@ -251,6 +252,18 @@ V8EscapableHandleScopeFromJsEscapableHandleScope(
return reinterpret_cast<EscapableHandleScopeWrapper*>(s);
}

static
napi_callback_scope JsCallbackScopeFromV8CallbackScope(
Copy link
Member

Choose a reason for hiding this comment

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

The name is a bit of a misnomer, isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's consistent with the naming of the other similar functions. We may want to change them all but I think that should be in a different PR.

node::CallbackScope* s) {
return reinterpret_cast<napi_callback_scope>(s);
}

static
node::CallbackScope* V8CallbackScopeFromJsCallbackScope(
napi_callback_scope s) {
return reinterpret_cast<node::CallbackScope*>(s);
}

//=== Conversion between V8 Handles and napi_value ========================

// This asserts v8::Local<> will always be implemented with a single
Expand Down Expand Up @@ -542,6 +555,7 @@ class CallbackWrapperBase : public CallbackWrapper {
napi_clear_last_error(env);

int open_handle_scopes = env->open_handle_scopes;
int open_callback_scopes = env->open_callback_scopes;

napi_value result = cb(env, cbinfo_wrapper);

Expand All @@ -550,6 +564,7 @@ class CallbackWrapperBase : public CallbackWrapper {
}

CHECK_EQ(env->open_handle_scopes, open_handle_scopes);
CHECK_EQ(env->open_callback_scopes, open_callback_scopes);

if (!env->last_exception.IsEmpty()) {
isolate->ThrowException(
Expand Down Expand Up @@ -909,7 +924,8 @@ const char* error_messages[] = {nullptr,
"An exception is pending",
"The async work item was cancelled",
"napi_escape_handle already called on scope",
"Invalid handle scope usage"};
"Invalid handle scope usage",
"Invalid callback scope usage"};

static inline napi_status napi_clear_last_error(napi_env env) {
env->last_error.error_code = napi_ok;
Expand Down Expand Up @@ -940,9 +956,9 @@ napi_status napi_get_last_error_info(napi_env env,
// We don't have a napi_status_last as this would result in an ABI
// change each time a message was added.
static_assert(
node::arraysize(error_messages) == napi_handle_scope_mismatch + 1,
node::arraysize(error_messages) == napi_callback_scope_mismatch + 1,
"Count of error messages must match count of error values");
CHECK_LE(env->last_error.error_code, napi_handle_scope_mismatch);
CHECK_LE(env->last_error.error_code, napi_callback_scope_mismatch);

// Wait until someone requests the last error information to fetch the error
// message string
Expand Down Expand Up @@ -2631,6 +2647,46 @@ napi_status napi_escape_handle(napi_env env,
return napi_set_last_error(env, napi_escape_called_twice);
}

napi_status napi_open_callback_scope(napi_env env,
napi_value resource_object,
napi_async_context async_context_handle,
napi_callback_scope* result) {
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
// JS exceptions.
CHECK_ENV(env);
CHECK_ARG(env, result);

v8::Local<v8::Context> context = env->isolate->GetCurrentContext();

node::async_context* node_async_context =
reinterpret_cast<node::async_context*>(async_context_handle);

v8::Local<v8::Object> resource;
CHECK_TO_OBJECT(env, context, resource, resource_object);

*result = v8impl::JsCallbackScopeFromV8CallbackScope(
new node::CallbackScope(env->isolate,
resource,
*node_async_context));

env->open_callback_scopes++;
return napi_clear_last_error(env);
}

napi_status napi_close_callback_scope(napi_env env, napi_callback_scope scope) {
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
// JS exceptions.
CHECK_ENV(env);
CHECK_ARG(env, scope);
if (env->open_callback_scopes == 0) {
return napi_callback_scope_mismatch;
}

env->open_callback_scopes--;
delete v8impl::V8CallbackScopeFromJsCallbackScope(scope);
return napi_clear_last_error(env);
}

napi_status napi_new_instance(napi_env env,
napi_value constructor,
size_t argc,
Expand Down
8 changes: 8 additions & 0 deletions src/node_api.h
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,14 @@ NAPI_EXTERN napi_status napi_escape_handle(napi_env env,
napi_value escapee,
napi_value* result);

NAPI_EXTERN napi_status napi_open_callback_scope(napi_env env,
napi_value resource_object,
napi_async_context context,
napi_callback_scope* result);

NAPI_EXTERN napi_status napi_close_callback_scope(napi_env env,
napi_callback_scope scope);

// Methods to support error handling
NAPI_EXTERN napi_status napi_throw(napi_env env, napi_value error);
NAPI_EXTERN napi_status napi_throw_error(napi_env env,
Expand Down
4 changes: 3 additions & 1 deletion src/node_api_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ typedef struct napi_value__ *napi_value;
typedef struct napi_ref__ *napi_ref;
typedef struct napi_handle_scope__ *napi_handle_scope;
typedef struct napi_escapable_handle_scope__ *napi_escapable_handle_scope;
typedef struct napi_callback_scope__ *napi_callback_scope;
typedef struct napi_callback_info__ *napi_callback_info;
typedef struct napi_async_context__ *napi_async_context;
typedef struct napi_async_work__ *napi_async_work;
Expand Down Expand Up @@ -70,7 +71,8 @@ typedef enum {
napi_pending_exception,
napi_cancelled,
napi_escape_called_twice,
napi_handle_scope_mismatch
napi_handle_scope_mismatch,
napi_callback_scope_mismatch
} napi_status;

typedef napi_value (*napi_callback)(napi_env env,
Expand Down
138 changes: 138 additions & 0 deletions test/addons-napi/test_callback_scope/binding.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
#include "node_api.h"
#include "uv.h"
#include "../common.h"

namespace {

// the test needs to fake out the async structure, so we need to use
// the raw structure here and then cast as done behind the scenes
// in napi calls.
struct async_context {
double async_id;
double trigger_async_id;
};


napi_value RunInCallbackScope(napi_env env, napi_callback_info info) {
size_t argc;
napi_value args[4];

NAPI_CALL(env, napi_get_cb_info(env, info, &argc, nullptr, nullptr, nullptr));
NAPI_ASSERT(env, argc ==4 , "Wrong number of arguments");
Copy link
Member

Choose a reason for hiding this comment

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

Space after ==.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will fix


NAPI_CALL(env, napi_get_cb_info(env, info, &argc, args, nullptr, nullptr));

napi_valuetype valuetype;
NAPI_CALL(env, napi_typeof(env, args[0], &valuetype));
NAPI_ASSERT(env, valuetype == napi_object,
"Wrong type of arguments. Expects an object as first argument.");

NAPI_CALL(env, napi_typeof(env, args[1], &valuetype));
NAPI_ASSERT(env, valuetype == napi_number,
"Wrong type of arguments. Expects a number as second argument.");

NAPI_CALL(env, napi_typeof(env, args[2], &valuetype));
NAPI_ASSERT(env, valuetype == napi_number,
"Wrong type of arguments. Expects a number as third argument.");

NAPI_CALL(env, napi_typeof(env, args[3], &valuetype));
NAPI_ASSERT(env, valuetype == napi_function,
"Wrong type of arguments. Expects a function as third argument.");

struct async_context context;
NAPI_CALL(env, napi_get_value_double(env, args[1], &context.async_id));
NAPI_CALL(env,
napi_get_value_double(env, args[2], &context.trigger_async_id));

napi_callback_scope scope = nullptr;
NAPI_CALL(
env,
napi_open_callback_scope(env,
args[0],
reinterpret_cast<napi_async_context>(&context),
&scope));

// if the function has an exception pending after the call that is ok
// so we don't use NAPI_CALL as we must close the callback scope regardless
napi_value result;
napi_status function_call_result =
napi_call_function(env, args[0], args[3], 0, nullptr, &result);
if (function_call_result != napi_ok) {
GET_AND_THROW_LAST_ERROR((env));
}

NAPI_CALL(env, napi_close_callback_scope(env, scope));

return result;
}

static napi_env shared_env = nullptr;
static napi_deferred deferred = nullptr;

static void Callback(uv_work_t* req, int ignored) {
napi_env env = shared_env;

napi_handle_scope handle_scope = nullptr;
NAPI_CALL_RETURN_VOID(env, napi_open_handle_scope(env, &handle_scope));

napi_value resource_name;
NAPI_CALL_RETURN_VOID(env, napi_create_string_utf8(
env, "test", NAPI_AUTO_LENGTH, &resource_name));
napi_async_context context;
NAPI_CALL_RETURN_VOID(env,
napi_async_init(env, nullptr, resource_name, &context));

napi_value resource_object;
NAPI_CALL_RETURN_VOID(env, napi_create_object(env, &resource_object));

napi_value undefined_value;
NAPI_CALL_RETURN_VOID(env, napi_get_undefined(env, &undefined_value));

napi_callback_scope scope = nullptr;
NAPI_CALL_RETURN_VOID(env, napi_open_callback_scope(env,
resource_object,
context,
&scope));

NAPI_CALL_RETURN_VOID(env,
napi_resolve_deferred(env, deferred, undefined_value));

NAPI_CALL_RETURN_VOID(env, napi_close_callback_scope(env, scope));

NAPI_CALL_RETURN_VOID(env, napi_close_handle_scope(env, handle_scope));
delete req;
}

napi_value TestResolveAsync(napi_env env, napi_callback_info info) {
napi_value promise;
if (deferred == nullptr) {
shared_env = env;
NAPI_CALL(env, napi_create_promise(env, &deferred, &promise));

uv_loop_t* loop = nullptr;
NAPI_CALL(env, napi_get_uv_event_loop(env, &loop));

uv_work_t* req = new uv_work_t;
Copy link
Member

Choose a reason for hiding this comment

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

Can you use new uv_work_t()? That stops coverity from complaining about uninitialized members.

Copy link
Member Author

Choose a reason for hiding this comment

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

will fix

uv_queue_work(loop,
req,
[](uv_work_t*) {},
Callback);
}
return promise;
}

napi_value Init(napi_env env, napi_value exports) {
napi_property_descriptor descriptors[] = {
DECLARE_NAPI_PROPERTY("runInCallbackScope", RunInCallbackScope),
DECLARE_NAPI_PROPERTY("testResolveAsync", TestResolveAsync)
};

NAPI_CALL(env, napi_define_properties(
env, exports, sizeof(descriptors) / sizeof(*descriptors), descriptors));

return exports;
}

} // namespace
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't the linter complain that you should be using // anonymous namespace?

Copy link
Member Author

Choose a reason for hiding this comment

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

no complaints from the linter but I can update.


NAPI_MODULE(NODE_GYP_MODULE_NAME, Init)
9 changes: 9 additions & 0 deletions test/addons-napi/test_callback_scope/binding.gyp
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
'targets': [
{
'target_name': 'binding',
'defines': [ 'V8_DEPRECATION_WARNINGS=1' ],
'sources': [ 'binding.cc' ]
}
]
}
29 changes: 29 additions & 0 deletions test/addons-napi/test_callback_scope/test-async-hooks.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
'use strict';

const common = require('../../common');
const assert = require('assert');
const async_hooks = require('async_hooks');

// The async_hook that we enable would register the process.emitWarning()
// call from loading the N-API addon as asynchronous activity because
// it contains a process.nextTick() call. Monkey patch it to be a no-op
// before we load the addon in order to avoid this.
process.emitWarning = () => {};

const { runInCallbackScope } = require(`./build/${common.buildType}/binding`);

let insideHook = false;
async_hooks.createHook({
before: common.mustCall((id) => {
assert.strictEqual(id, 1000);
insideHook = true;
}),
after: common.mustCall((id) => {
assert.strictEqual(id, 1000);
insideHook = false;
})
}).enable();

runInCallbackScope({}, 1000, 1000, () => {
assert(insideHook);
});
13 changes: 13 additions & 0 deletions test/addons-napi/test_callback_scope/test-resolve-async.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
'use strict';

const common = require('../../common');
const assert = require('assert');
const { testResolveAsync } = require(`./build/${common.buildType}/binding`);

let called = false;
testResolveAsync().then(common.mustCall(() => {
called = true;
}));

setTimeout(common.mustCall(() => { assert(called); }),
common.platformTimeout(20));
Loading