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: nodejs#21688 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Kyle Farnung <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gus Caplan <[email protected]>
gabrielschulhof · Aug 16, 2018 · 3078ccf · 3078ccf
1 parent 69efa9f
commit 3078ccf
Show file tree

Hide file tree

Showing 3 changed files with 104 additions and 10 deletions.
diff --git a/src/node_api.cc b/src/node_api.cc
@@ -1023,11 +1023,11 @@ napi_status napi_create_function(napi_env env,
 
   RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure);
 
-  v8::Local<v8::FunctionTemplate> tpl = v8::FunctionTemplate::New(
-      isolate, v8impl::FunctionCallbackWrapper::Invoke, cbdata);
-
   v8::Local<v8::Context> context = isolate->GetCurrentContext();
-  v8::MaybeLocal<v8::Function> maybe_function = tpl->GetFunction(context);
+  v8::MaybeLocal<v8::Function> 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());
@@ -1489,13 +1489,17 @@ napi_status napi_define_properties(napi_env env,
       v8::Local<v8::Value> 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<v8::Function> maybe_fn =
+          v8::Function::New(context,
+                            v8impl::FunctionCallbackWrapper::Invoke,
+                            cbdata);
 
-      v8::Local<v8::FunctionTemplate> 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
@@ -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
@@ -30,7 +30,79 @@ napi_value TestFunctionName(napi_env env, napi_callback_info info) {
   return NULL;
 }
 
-napi_value Init(napi_env env, napi_value exports) {
+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(
       env, NULL, NAPI_AUTO_LENGTH, TestCallFunction, NULL, &fn1));
@@ -43,9 +115,21 @@ 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;
 }