From 978d89f5e3cf238a0e42303acc9493993ede9c98 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Fri, 6 Jul 2018 10:47:48 -0400 Subject: [PATCH] n-api: create functions directly Avoid using `v8::FunctionTemplate::New()` when using `v8::Function::New()` suffices. This ensures that individual functions can be gc-ed and that functions can be created dynamically without running out of memory. PR-URL: https://github.com/nodejs/node/pull/21688 Reviewed-By: Anna Henningsen Reviewed-By: Kyle Farnung Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: Yang Guo Reviewed-By: Colin Ihrig Reviewed-By: Gus Caplan --- src/node_api.cc | 20 +++-- test/addons-napi/test_function/test.js | 8 +- .../addons-napi/test_function/test_function.c | 84 +++++++++++++++++++ 3 files changed, 103 insertions(+), 9 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index 2b14617175c167..1d42435264cfec 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -1025,11 +1025,11 @@ napi_status napi_create_function(napi_env env, RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure); - v8::Local tpl = v8::FunctionTemplate::New( - isolate, v8impl::FunctionCallbackWrapper::Invoke, cbdata); - v8::Local context = isolate->GetCurrentContext(); - v8::MaybeLocal maybe_function = tpl->GetFunction(context); + v8::MaybeLocal maybe_function = + v8::Function::New(context, + v8impl::FunctionCallbackWrapper::Invoke, + cbdata); CHECK_MAYBE_EMPTY(env, maybe_function, napi_generic_failure); return_value = scope.Escape(maybe_function.ToLocalChecked()); @@ -1491,13 +1491,17 @@ napi_status napi_define_properties(napi_env env, v8::Local cbdata = v8impl::CreateFunctionCallbackData(env, p->method, p->data); - RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure); + CHECK_MAYBE_EMPTY(env, cbdata, napi_generic_failure); + + v8::MaybeLocal maybe_fn = + v8::Function::New(context, + v8impl::FunctionCallbackWrapper::Invoke, + cbdata); - v8::Local t = v8::FunctionTemplate::New( - isolate, v8impl::FunctionCallbackWrapper::Invoke, cbdata); + CHECK_MAYBE_EMPTY(env, maybe_fn, napi_generic_failure); auto define_maybe = obj->DefineOwnProperty( - context, property_name, t->GetFunction(), attributes); + context, property_name, maybe_fn.ToLocalChecked(), attributes); if (!define_maybe.FromMaybe(false)) { return napi_set_last_error(env, napi_generic_failure); diff --git a/test/addons-napi/test_function/test.js b/test/addons-napi/test_function/test.js index 752e9965b23039..6f0f1681752a2c 100644 --- a/test/addons-napi/test_function/test.js +++ b/test/addons-napi/test_function/test.js @@ -1,11 +1,12 @@ 'use strict'; +// Flags: --expose-gc + const common = require('../../common'); const assert = require('assert'); // testing api calls for function const test_function = require(`./build/${common.buildType}/test_function`); - function func1() { return 1; } @@ -29,3 +30,8 @@ assert.strictEqual(test_function.TestCall(func4, 1), 2); assert.strictEqual(test_function.TestName.name, 'Name'); assert.strictEqual(test_function.TestNameShort.name, 'Name_'); + +let tracked_function = test_function.MakeTrackedFunction(common.mustCall()); +assert(!!tracked_function); +tracked_function = null; +global.gc(); diff --git a/test/addons-napi/test_function/test_function.c b/test/addons-napi/test_function/test_function.c index 2c361933cfa071..068999a6e5bc5c 100644 --- a/test/addons-napi/test_function/test_function.c +++ b/test/addons-napi/test_function/test_function.c @@ -30,6 +30,78 @@ static napi_value TestFunctionName(napi_env env, napi_callback_info info) { return NULL; } +static void finalize_function(napi_env env, void* data, void* hint) { + napi_ref ref = data; + + // Retrieve the JavaScript undefined value. + napi_value undefined; + NAPI_CALL_RETURN_VOID(env, napi_get_undefined(env, &undefined)); + + // Retrieve the JavaScript function we must call. + napi_value js_function; + NAPI_CALL_RETURN_VOID(env, napi_get_reference_value(env, ref, &js_function)); + + // Call the JavaScript function to indicate that the generated JavaScript + // function is about to be gc-ed. + NAPI_CALL_RETURN_VOID(env, napi_call_function(env, + undefined, + js_function, + 0, + NULL, + NULL)); + + // Destroy the persistent reference to the function we just called so as to + // properly clean up. + NAPI_CALL_RETURN_VOID(env, napi_delete_reference(env, ref)); +} + +static napi_value MakeTrackedFunction(napi_env env, napi_callback_info info) { + size_t argc = 1; + napi_value js_finalize_cb; + napi_valuetype arg_type; + + // Retrieve and validate from the arguments the function we will use to + // indicate to JavaScript that the function we are about to create is about to + // be gc-ed. + NAPI_CALL(env, napi_get_cb_info(env, + info, + &argc, + &js_finalize_cb, + NULL, + NULL)); + NAPI_ASSERT(env, argc == 1, "Wrong number of arguments"); + NAPI_CALL(env, napi_typeof(env, js_finalize_cb, &arg_type)); + NAPI_ASSERT(env, arg_type == napi_function, "Argument must be a function"); + + // Dynamically create a function. + napi_value result; + NAPI_CALL(env, napi_create_function(env, + "TrackedFunction", + NAPI_AUTO_LENGTH, + TestFunctionName, + NULL, + &result)); + + // Create a strong reference to the function we will call when the tracked + // function is about to be gc-ed. + napi_ref js_finalize_cb_ref; + NAPI_CALL(env, napi_create_reference(env, + js_finalize_cb, + 1, + &js_finalize_cb_ref)); + + // Attach a finalizer to the dynamically created function and pass it the + // strong reference we created in the previous step. + NAPI_CALL(env, napi_wrap(env, + result, + js_finalize_cb_ref, + finalize_function, + NULL, + NULL)); + + return result; +} + static napi_value Init(napi_env env, napi_value exports) { napi_value fn1; NAPI_CALL(env, napi_create_function( @@ -43,9 +115,21 @@ static napi_value Init(napi_env env, napi_value exports) { NAPI_CALL(env, napi_create_function( env, "Name_extra", 5, TestFunctionName, NULL, &fn3)); + napi_value fn4; + NAPI_CALL(env, napi_create_function(env, + "MakeTrackedFunction", + NAPI_AUTO_LENGTH, + MakeTrackedFunction, + NULL, + &fn4)); + NAPI_CALL(env, napi_set_named_property(env, exports, "TestCall", fn1)); NAPI_CALL(env, napi_set_named_property(env, exports, "TestName", fn2)); NAPI_CALL(env, napi_set_named_property(env, exports, "TestNameShort", fn3)); + NAPI_CALL(env, napi_set_named_property(env, + exports, + "MakeTrackedFunction", + fn4)); return exports; }