From a0f449284bdd3a414ca071ae4760dd6fceb8e8d7 Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Tue, 12 Dec 2017 19:37:40 -0500 Subject: [PATCH 1/3] n-api: add methods to open/close callback scope Add support for the following methods; napi_open_callback_scope napi_close_callback_scope These are needed when running asynchronous methods directly using uv. --- doc/api/n-api.md | 38 +++++ src/node_api.cc | 62 +++++++- src/node_api.h | 8 + src/node_api_types.h | 4 +- .../test_callback_scope/binding.cc | 138 ++++++++++++++++++ .../test_callback_scope/binding.gyp | 9 ++ .../test_callback_scope/test-async-hooks.js | 29 ++++ .../test_callback_scope/test-resolve-async.js | 13 ++ test/addons-napi/test_callback_scope/test.js | 17 +++ 9 files changed, 314 insertions(+), 4 deletions(-) create mode 100644 test/addons-napi/test_callback_scope/binding.cc create mode 100644 test/addons-napi/test_callback_scope/binding.gyp create mode 100644 test/addons-napi/test_callback_scope/test-async-hooks.js create mode 100644 test/addons-napi/test_callback_scope/test-resolve-async.js create mode 100644 test/addons-napi/test_callback_scope/test.js diff --git a/doc/api/n-api.md b/doc/api/n-api.md index 05e799cc0e550b..93dd458b89b18a 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -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* + +```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* + +```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 @@ -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 @@ -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 diff --git a/src/node_api.cc b/src/node_api.cc index 78841d14882eeb..71159005c9e5cd 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -44,6 +44,7 @@ struct napi_env__ { v8::Persistent accessor_data_template; napi_extended_error_info last_error; int open_handle_scopes = 0; + int open_callback_scopes = 0; uv_loop_t* loop = nullptr; }; @@ -251,6 +252,18 @@ V8EscapableHandleScopeFromJsEscapableHandleScope( return reinterpret_cast(s); } +static +napi_callback_scope JsCallbackScopeFromV8CallbackScope( + node::CallbackScope* s) { + return reinterpret_cast(s); +} + +static +node::CallbackScope* V8CallbackScopeFromJsCallbackScope( + napi_callback_scope s) { + return reinterpret_cast(s); +} + //=== Conversion between V8 Handles and napi_value ======================== // This asserts v8::Local<> will always be implemented with a single @@ -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); @@ -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( @@ -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; @@ -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 @@ -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 context = env->isolate->GetCurrentContext(); + + node::async_context* node_async_context = + reinterpret_cast(async_context_handle); + + v8::Local 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, diff --git a/src/node_api.h b/src/node_api.h index c7527051694537..e9b3645e404530 100644 --- a/src/node_api.h +++ b/src/node_api.h @@ -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, diff --git a/src/node_api_types.h b/src/node_api_types.h index 230c1f4ae3446f..76f38802e83e2e 100644 --- a/src/node_api_types.h +++ b/src/node_api_types.h @@ -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; @@ -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, diff --git a/test/addons-napi/test_callback_scope/binding.cc b/test/addons-napi/test_callback_scope/binding.cc new file mode 100644 index 00000000000000..17b76b9447ba91 --- /dev/null +++ b/test/addons-napi/test_callback_scope/binding.cc @@ -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"); + + 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(&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; + 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 + +NAPI_MODULE(NODE_GYP_MODULE_NAME, Init) diff --git a/test/addons-napi/test_callback_scope/binding.gyp b/test/addons-napi/test_callback_scope/binding.gyp new file mode 100644 index 00000000000000..7ede63d94a0d77 --- /dev/null +++ b/test/addons-napi/test_callback_scope/binding.gyp @@ -0,0 +1,9 @@ +{ + 'targets': [ + { + 'target_name': 'binding', + 'defines': [ 'V8_DEPRECATION_WARNINGS=1' ], + 'sources': [ 'binding.cc' ] + } + ] +} diff --git a/test/addons-napi/test_callback_scope/test-async-hooks.js b/test/addons-napi/test_callback_scope/test-async-hooks.js new file mode 100644 index 00000000000000..1a11bf60398f9b --- /dev/null +++ b/test/addons-napi/test_callback_scope/test-async-hooks.js @@ -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); +}); diff --git a/test/addons-napi/test_callback_scope/test-resolve-async.js b/test/addons-napi/test_callback_scope/test-resolve-async.js new file mode 100644 index 00000000000000..e9f4b9044c0154 --- /dev/null +++ b/test/addons-napi/test_callback_scope/test-resolve-async.js @@ -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)); diff --git a/test/addons-napi/test_callback_scope/test.js b/test/addons-napi/test_callback_scope/test.js new file mode 100644 index 00000000000000..2f2efe5f47b98a --- /dev/null +++ b/test/addons-napi/test_callback_scope/test.js @@ -0,0 +1,17 @@ +'use strict'; + +const common = require('../../common'); +const assert = require('assert'); +const { runInCallbackScope } = require(`./build/${common.buildType}/binding`); + +assert.strictEqual(runInCallbackScope({}, 0, 0, () => 42), 42); + +{ + process.once('uncaughtException', common.mustCall((err) => { + assert.strictEqual(err.message, 'foo'); + })); + + runInCallbackScope({}, 0, 0, () => { + throw new Error('foo'); + }); +} From e9200b41957c35f4d774468ee97d3c00145329ea Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Mon, 5 Feb 2018 11:56:55 -0500 Subject: [PATCH 2/3] squash: address comments --- test/addons-napi/test_callback_scope/binding.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/addons-napi/test_callback_scope/binding.cc b/test/addons-napi/test_callback_scope/binding.cc index 17b76b9447ba91..27c880a3b3220f 100644 --- a/test/addons-napi/test_callback_scope/binding.cc +++ b/test/addons-napi/test_callback_scope/binding.cc @@ -18,7 +18,7 @@ napi_value RunInCallbackScope(napi_env env, napi_callback_info info) { 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"); + NAPI_ASSERT(env, argc == 4 , "Wrong number of arguments"); NAPI_CALL(env, napi_get_cb_info(env, info, &argc, args, nullptr, nullptr)); @@ -112,7 +112,7 @@ napi_value TestResolveAsync(napi_env env, napi_callback_info info) { uv_loop_t* loop = nullptr; NAPI_CALL(env, napi_get_uv_event_loop(env, &loop)); - uv_work_t* req = new uv_work_t; + uv_work_t* req = new uv_work_t(); uv_queue_work(loop, req, [](uv_work_t*) {}, @@ -133,6 +133,6 @@ napi_value Init(napi_env env, napi_value exports) { return exports; } -} // namespace +} // anonymous namespace NAPI_MODULE(NODE_GYP_MODULE_NAME, Init) From a1ea4c8a7dbae6654ae2cb99fc81eb5c36aa707b Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Mon, 5 Feb 2018 13:12:36 -0500 Subject: [PATCH 3/3] squash: fix valgrind warnings --- test/addons-napi/test_callback_scope/binding.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/addons-napi/test_callback_scope/binding.cc b/test/addons-napi/test_callback_scope/binding.cc index 27c880a3b3220f..e6631b6ac7bb52 100644 --- a/test/addons-napi/test_callback_scope/binding.cc +++ b/test/addons-napi/test_callback_scope/binding.cc @@ -54,7 +54,7 @@ napi_value RunInCallbackScope(napi_env env, napi_callback_info info) { // 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_value result = nullptr; napi_status function_call_result = napi_call_function(env, args[0], args[3], 0, nullptr, &result); if (function_call_result != napi_ok) { @@ -104,7 +104,7 @@ static void Callback(uv_work_t* req, int ignored) { } napi_value TestResolveAsync(napi_env env, napi_callback_info info) { - napi_value promise; + napi_value promise = nullptr; if (deferred == nullptr) { shared_env = env; NAPI_CALL(env, napi_create_promise(env, &deferred, &promise));