From 801c1469bb88df478f5e104714c6bd4282a02fd6 Mon Sep 17 00:00:00 2001 From: Jason Ginchereau Date: Thu, 20 Apr 2017 13:01:07 -0700 Subject: [PATCH 1/2] n-api: Reference and external tests - Add a test project to addons-napi that covers the N-API reference and external APIs - Fix a bug in napi_typeof that was found by the new tests --- src/node_api.cc | 6 +- test/addons-napi/common.h | 4 + test/addons-napi/test_reference/binding.gyp | 8 + test/addons-napi/test_reference/test.js | 72 +++++++++ .../test_reference/test_reference.c | 149 ++++++++++++++++++ 5 files changed, 237 insertions(+), 2 deletions(-) create mode 100644 test/addons-napi/test_reference/binding.gyp create mode 100644 test/addons-napi/test_reference/test.js create mode 100644 test/addons-napi/test_reference/test_reference.c diff --git a/src/node_api.cc b/src/node_api.cc index f59dcae4e11e2d..3b3ed0656a5a3c 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -1431,6 +1431,10 @@ napi_status napi_typeof(napi_env env, // This test has to come before IsObject because IsFunction // implies IsObject *result = napi_function; + } else if (v->IsExternal()) { + // This test has to come before IsObject because IsExternal + // implies IsObject + *result = napi_external; } else if (v->IsObject()) { *result = napi_object; } else if (v->IsBoolean()) { @@ -1441,8 +1445,6 @@ napi_status napi_typeof(napi_env env, *result = napi_symbol; } else if (v->IsNull()) { *result = napi_null; - } else if (v->IsExternal()) { - *result = napi_external; } else { // Should not get here unless V8 has added some new kind of value. return napi_set_last_error(env, napi_invalid_arg); diff --git a/test/addons-napi/common.h b/test/addons-napi/common.h index b06ece6e39c7d6..cc8cd6a5780dd2 100644 --- a/test/addons-napi/common.h +++ b/test/addons-napi/common.h @@ -50,3 +50,7 @@ #define DECLARE_NAPI_PROPERTY(name, func) \ { (name), 0, (func), 0, 0, 0, napi_default, 0 } + +#define DECLARE_NAPI_GETTER(name, func) \ + { (name), 0, 0, (func), 0, 0, napi_default, 0 } + diff --git a/test/addons-napi/test_reference/binding.gyp b/test/addons-napi/test_reference/binding.gyp new file mode 100644 index 00000000000000..3a6d69a65c81f5 --- /dev/null +++ b/test/addons-napi/test_reference/binding.gyp @@ -0,0 +1,8 @@ +{ + "targets": [ + { + "target_name": "test_reference", + "sources": [ "test_reference.c" ] + } + ] +} diff --git a/test/addons-napi/test_reference/test.js b/test/addons-napi/test_reference/test.js new file mode 100644 index 00000000000000..56b09d1c01c615 --- /dev/null +++ b/test/addons-napi/test_reference/test.js @@ -0,0 +1,72 @@ +'use strict'; +// Flags: --expose-gc + +const common = require('../../common'); +const assert = require('assert'); + +const test_reference = require(`./build/${common.buildType}/test_reference`); + +// This test script uses external values with finalizer callbacks +// in order to track when values get garbage-collected. Each invocation +// of a finalizer callback increments the finalizeCount property. +assert.strictEqual(0, test_reference.finalizeCount); + +// External value without a finalizer +let value = test_reference.createExternal(); +assert.strictEqual(typeof value, 'object'); +test_reference.checkExternal(value); +value = null; +global.gc(); +assert.strictEqual(0, test_reference.finalizeCount); + +// External value with a finalizer +value = test_reference.createExternalWithFinalize(); +assert.strictEqual(typeof value, 'object'); +test_reference.checkExternal(value); +value = null; +global.gc(); +assert.strictEqual(1, test_reference.finalizeCount); + +// Weak reference +value = test_reference.createExternalWithFinalize(); +test_reference.createReference(value, 0); +assert.strictEqual(test_reference.referenceValue, value); +value = null; +global.gc(); // Value should be GC'd because there is only a weak ref +assert.strictEqual(test_reference.referenceValue, undefined); +assert.strictEqual(2, test_reference.finalizeCount); +test_reference.deleteReference(); + +// Strong reference +value = test_reference.createExternalWithFinalize(); +test_reference.createReference(value, 1); +assert.strictEqual(test_reference.referenceValue, value); +value = null; +global.gc(); // Value should NOT be GC'd because there is a strong ref +assert.strictEqual(2, test_reference.finalizeCount); +test_reference.deleteReference(); +global.gc(); // Value should be GC'd because the strong ref was deleted +assert.strictEqual(3, test_reference.finalizeCount); + +// Strong reference, increment then decrement to weak reference +value = test_reference.createExternalWithFinalize(); +test_reference.createReference(value, 1); +value = null; +global.gc(); // Value should NOT be GC'd because there is a strong ref +assert.strictEqual(3, test_reference.finalizeCount); + +assert.strictEqual(test_reference.incrementRefcount(), 2); +global.gc(); // Value should NOT be GC'd because there is a strong ref +assert.strictEqual(3, test_reference.finalizeCount); + +assert.strictEqual(test_reference.decrementRefcount(), 1); +global.gc(); // Value should NOT be GC'd because there is a strong ref +assert.strictEqual(3, test_reference.finalizeCount); + +assert.strictEqual(test_reference.decrementRefcount(), 0); +global.gc(); // Value should be GC'd because the ref is now weak! +assert.strictEqual(4, test_reference.finalizeCount); + +test_reference.deleteReference(); +global.gc(); // Value was already GC'd +assert.strictEqual(4, test_reference.finalizeCount); diff --git a/test/addons-napi/test_reference/test_reference.c b/test/addons-napi/test_reference/test_reference.c new file mode 100644 index 00000000000000..383fdcef750df0 --- /dev/null +++ b/test/addons-napi/test_reference/test_reference.c @@ -0,0 +1,149 @@ +#include +#include "../common.h" +#include + +static int test_value = 1; +static int finalize_count = 0; +static napi_ref test_reference = NULL; + +napi_value GetFinalizeCount(napi_env env, napi_callback_info info) { + napi_value result; + NAPI_CALL(env, napi_create_number(env, finalize_count, &result)); + return result; +} + +void FinalizeExternal(napi_env env, void* data, void* hint) { + free(data); + finalize_count++; +} + +napi_value CreateExternal(napi_env env, napi_callback_info info) { + int* data = &test_value; + + napi_value result; + NAPI_CALL(env, + napi_create_external(env, + data, + NULL, /* finalize_cb */ + NULL, /* finalize_hint */ + &result)); + return result; +} + +napi_value CreateExternalWithFinalize(napi_env env, napi_callback_info info) { + int* data = malloc(sizeof(int)); + *data = test_value; + + napi_value result; + NAPI_CALL(env, + napi_create_external(env, + data, + FinalizeExternal, + NULL, /* finalize_hint */ + &result)); + return result; +} + +napi_value CheckExternal(napi_env env, napi_callback_info info) { + size_t argc = 1; + napi_value arg; + NAPI_CALL(env, napi_get_cb_info(env, info, &argc, &arg, NULL, NULL)); + + NAPI_ASSERT(env, argc == 1, "Expected one argument."); + + napi_valuetype argtype; + NAPI_CALL(env, napi_typeof(env, arg, &argtype)); + + NAPI_ASSERT(env, argtype == napi_external, "Expected an external value.") + + int* data; + NAPI_CALL(env, napi_get_value_external(env, arg, &data)); + + NAPI_ASSERT(env, data != NULL && *data == test_value, + "An external data value of 1 was expected.") + + return NULL; +} + +napi_value CreateReference(napi_env env, napi_callback_info info) { + NAPI_ASSERT(env, test_reference == NULL, + "The test allows only one reference at a time."); + + size_t argc = 2; + napi_value args[2]; + NAPI_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL)); + NAPI_ASSERT(env, argc == 2, "Expected two arguments."); + + uint32_t initial_refcount; + NAPI_CALL(env, napi_get_value_uint32(env, args[1], &initial_refcount)); + + NAPI_CALL(env, + napi_create_reference(env, args[0], initial_refcount, &test_reference)); + + NAPI_ASSERT(env, test_reference != NULL, + "A reference should have been created."); + + return NULL; +} + +napi_value DeleteReference(napi_env env, napi_callback_info info) { + NAPI_ASSERT(env, test_reference != NULL, + "A reference must have been created."); + + NAPI_CALL(env, napi_delete_reference(env, test_reference)); + test_reference = NULL; + return NULL; +} + +napi_value IncrementRefcount(napi_env env, napi_callback_info info) { + NAPI_ASSERT(env, test_reference != NULL, + "A reference must have been created."); + + uint32_t refcount; + NAPI_CALL(env, napi_reference_ref(env, test_reference, &refcount)); + + napi_value result; + NAPI_CALL(env, napi_create_number(env, refcount, &result)); + return result; +} + +napi_value DecrementRefcount(napi_env env, napi_callback_info info) { + NAPI_ASSERT(env, test_reference != NULL, + "A reference must have been created."); + + uint32_t refcount; + NAPI_CALL(env, napi_reference_unref(env, test_reference, &refcount)); + + napi_value result; + NAPI_CALL(env, napi_create_number(env, refcount, &result)); + return result; +} + +napi_value GetReferenceValue(napi_env env, napi_callback_info info) { + NAPI_ASSERT(env, test_reference != NULL, + "A reference must have been created."); + + napi_value result; + NAPI_CALL(env, napi_get_reference_value(env, test_reference, &result)); + return result; +} + +void Init(napi_env env, napi_value exports, napi_value module, void* priv) { + napi_property_descriptor descriptors[] = { + DECLARE_NAPI_GETTER("finalizeCount", GetFinalizeCount), + DECLARE_NAPI_PROPERTY("createExternal", CreateExternal), + DECLARE_NAPI_PROPERTY("createExternalWithFinalize", + CreateExternalWithFinalize), + DECLARE_NAPI_PROPERTY("checkExternal", CheckExternal), + DECLARE_NAPI_PROPERTY("createReference", CreateReference), + DECLARE_NAPI_PROPERTY("deleteReference", DeleteReference), + DECLARE_NAPI_PROPERTY("incrementRefcount", IncrementRefcount), + DECLARE_NAPI_PROPERTY("decrementRefcount", DecrementRefcount), + DECLARE_NAPI_GETTER("referenceValue", GetReferenceValue), + }; + + NAPI_CALL_RETURN_VOID(env, napi_define_properties( + env, exports, sizeof(descriptors) / sizeof(*descriptors), descriptors)); +} + +NAPI_MODULE(addon, Init) From caf86eddf3303bef65028745b9fd95107bb861df Mon Sep 17 00:00:00 2001 From: Jason Ginchereau Date: Tue, 25 Apr 2017 13:19:05 -0700 Subject: [PATCH 2/2] Updates for review feedback - Make test cases more isolated with block scoping and reset finalize count - Fix order of actual, expected in asserts --- test/addons-napi/test_reference/test.js | 119 ++++++++++-------- .../test_reference/test_reference.c | 4 + 2 files changed, 71 insertions(+), 52 deletions(-) diff --git a/test/addons-napi/test_reference/test.js b/test/addons-napi/test_reference/test.js index 56b09d1c01c615..ddfec58f1f9d7d 100644 --- a/test/addons-napi/test_reference/test.js +++ b/test/addons-napi/test_reference/test.js @@ -9,64 +9,79 @@ const test_reference = require(`./build/${common.buildType}/test_reference`); // This test script uses external values with finalizer callbacks // in order to track when values get garbage-collected. Each invocation // of a finalizer callback increments the finalizeCount property. -assert.strictEqual(0, test_reference.finalizeCount); +assert.strictEqual(test_reference.finalizeCount, 0); -// External value without a finalizer -let value = test_reference.createExternal(); -assert.strictEqual(typeof value, 'object'); -test_reference.checkExternal(value); -value = null; -global.gc(); -assert.strictEqual(0, test_reference.finalizeCount); +{ + // External value without a finalizer + let value = test_reference.createExternal(); + assert.strictEqual(test_reference.finalizeCount, 0); + assert.strictEqual(typeof value, 'object'); + test_reference.checkExternal(value); + value = null; + global.gc(); + assert.strictEqual(test_reference.finalizeCount, 0); +} -// External value with a finalizer -value = test_reference.createExternalWithFinalize(); -assert.strictEqual(typeof value, 'object'); -test_reference.checkExternal(value); -value = null; -global.gc(); -assert.strictEqual(1, test_reference.finalizeCount); +{ + // External value with a finalizer + let value = test_reference.createExternalWithFinalize(); + assert.strictEqual(test_reference.finalizeCount, 0); + assert.strictEqual(typeof value, 'object'); + test_reference.checkExternal(value); + value = null; + global.gc(); + assert.strictEqual(test_reference.finalizeCount, 1); +} -// Weak reference -value = test_reference.createExternalWithFinalize(); -test_reference.createReference(value, 0); -assert.strictEqual(test_reference.referenceValue, value); -value = null; -global.gc(); // Value should be GC'd because there is only a weak ref -assert.strictEqual(test_reference.referenceValue, undefined); -assert.strictEqual(2, test_reference.finalizeCount); -test_reference.deleteReference(); +{ + // Weak reference + let value = test_reference.createExternalWithFinalize(); + assert.strictEqual(test_reference.finalizeCount, 0); + test_reference.createReference(value, 0); + assert.strictEqual(test_reference.referenceValue, value); + value = null; + global.gc(); // Value should be GC'd because there is only a weak ref + assert.strictEqual(test_reference.referenceValue, undefined); + assert.strictEqual(test_reference.finalizeCount, 1); + test_reference.deleteReference(); +} -// Strong reference -value = test_reference.createExternalWithFinalize(); -test_reference.createReference(value, 1); -assert.strictEqual(test_reference.referenceValue, value); -value = null; -global.gc(); // Value should NOT be GC'd because there is a strong ref -assert.strictEqual(2, test_reference.finalizeCount); -test_reference.deleteReference(); -global.gc(); // Value should be GC'd because the strong ref was deleted -assert.strictEqual(3, test_reference.finalizeCount); +{ + // Strong reference + let value = test_reference.createExternalWithFinalize(); + assert.strictEqual(test_reference.finalizeCount, 0); + test_reference.createReference(value, 1); + assert.strictEqual(test_reference.referenceValue, value); + value = null; + global.gc(); // Value should NOT be GC'd because there is a strong ref + assert.strictEqual(test_reference.finalizeCount, 0); + test_reference.deleteReference(); + global.gc(); // Value should be GC'd because the strong ref was deleted + assert.strictEqual(test_reference.finalizeCount, 1); +} -// Strong reference, increment then decrement to weak reference -value = test_reference.createExternalWithFinalize(); -test_reference.createReference(value, 1); -value = null; -global.gc(); // Value should NOT be GC'd because there is a strong ref -assert.strictEqual(3, test_reference.finalizeCount); +{ + // Strong reference, increment then decrement to weak reference + let value = test_reference.createExternalWithFinalize(); + assert.strictEqual(test_reference.finalizeCount, 0); + test_reference.createReference(value, 1); + value = null; + global.gc(); // Value should NOT be GC'd because there is a strong ref + assert.strictEqual(test_reference.finalizeCount, 0); -assert.strictEqual(test_reference.incrementRefcount(), 2); -global.gc(); // Value should NOT be GC'd because there is a strong ref -assert.strictEqual(3, test_reference.finalizeCount); + assert.strictEqual(test_reference.incrementRefcount(), 2); + global.gc(); // Value should NOT be GC'd because there is a strong ref + assert.strictEqual(test_reference.finalizeCount, 0); -assert.strictEqual(test_reference.decrementRefcount(), 1); -global.gc(); // Value should NOT be GC'd because there is a strong ref -assert.strictEqual(3, test_reference.finalizeCount); + assert.strictEqual(test_reference.decrementRefcount(), 1); + global.gc(); // Value should NOT be GC'd because there is a strong ref + assert.strictEqual(test_reference.finalizeCount, 0); -assert.strictEqual(test_reference.decrementRefcount(), 0); -global.gc(); // Value should be GC'd because the ref is now weak! -assert.strictEqual(4, test_reference.finalizeCount); + assert.strictEqual(test_reference.decrementRefcount(), 0); + global.gc(); // Value should be GC'd because the ref is now weak! + assert.strictEqual(test_reference.finalizeCount, 1); -test_reference.deleteReference(); -global.gc(); // Value was already GC'd -assert.strictEqual(4, test_reference.finalizeCount); + test_reference.deleteReference(); + global.gc(); // Value was already GC'd + assert.strictEqual(test_reference.finalizeCount, 1); +} diff --git a/test/addons-napi/test_reference/test_reference.c b/test/addons-napi/test_reference/test_reference.c index 383fdcef750df0..1a238a560aac53 100644 --- a/test/addons-napi/test_reference/test_reference.c +++ b/test/addons-napi/test_reference/test_reference.c @@ -27,6 +27,8 @@ napi_value CreateExternal(napi_env env, napi_callback_info info) { NULL, /* finalize_cb */ NULL, /* finalize_hint */ &result)); + + finalize_count = 0; return result; } @@ -41,6 +43,8 @@ napi_value CreateExternalWithFinalize(napi_env env, napi_callback_info info) { FinalizeExternal, NULL, /* finalize_hint */ &result)); + + finalize_count = 0; return result; }