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: Reference and external tests #12551

Closed
wants to merge 2 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
6 changes: 4 additions & 2 deletions src/node_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand All @@ -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);
Expand Down
4 changes: 4 additions & 0 deletions test/addons-napi/common.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 }

8 changes: 8 additions & 0 deletions test/addons-napi/test_reference/binding.gyp
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"targets": [
{
"target_name": "test_reference",
"sources": [ "test_reference.c" ]
}
]
}
72 changes: 72 additions & 0 deletions test/addons-napi/test_reference/test.js
Original file line number Diff line number Diff line change
@@ -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`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you stick to camelCase in JavaScript code.

Copy link
Member Author

Choose a reason for hiding this comment

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

The underscore matches the name of the module. I was trying to follow precedent in node.js JavaScript code, often with the child_process module, for example at https://github.com/nodejs/node/blob/master/test/common.js#L28


// 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change the order of the arguments to actual, expected in strictEqual() throughout the test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, good catch. I'm used to some other testing frameworks that use the opposite order.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in caf86ed


// External value without a finalizer
let value = test_reference.createExternal();
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 turn these groups into {} blocks and use const where that works? That makes it a bit easier to see how different parts of a test (don’t) interact :)

Copy link
Member Author

Choose a reason for hiding this comment

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

const won't work because value must later be set to null in order to allow it to be garbage-collected.

... unless the scope of the { } blocks is narrowed to just around where each value is live. But then I think that would actually hurt readability because the blocks would not correspond to the logical test cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also I think allowing value to go out of scope to enable GC is less obvious than assigning null.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @addaleax. Unless necessary for the purposes of the test, we've been moving toward using block scopes in the tests for better isolation.

Copy link
Member

Choose a reason for hiding this comment

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

@jasongin Yea, let seems fine here. You can still do both assigning null and using block scopes, if you think that’s more readable (I would say it is)

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I'm adding block scopes around each test case, keeping the null assignment within.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in caf86ed

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);
149 changes: 149 additions & 0 deletions test/addons-napi/test_reference/test_reference.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
#include <node_api.h>
#include "../common.h"
#include <stdlib.h>

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)