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.

Sign up for GitHub

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

test: use async/await for gc #773

Closed
wants to merge 5 commits into from
Closed

test: use async/await for gc #773

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5d36882
test: string tests together
Jul 25, 2020
506b995
remove async/await pairs where a promise could be returned
Jul 26, 2020
a152387
remove unused reject()
Jul 26, 2020
5ba003c
remove race from tsfn sum test
Jul 27, 2020
7292064
remove cancel test
Jul 27, 2020
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
47 changes: 28 additions & 19 deletions test/addon_data.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,29 +5,37 @@ const { spawn } = require('child_process');
const readline = require('readline');
const path = require('path');

test(path.resolve(__dirname, `./build/${buildType}/binding.node`));
test(path.resolve(__dirname, `./build/${buildType}/binding_noexcept.node`));
module.exports =
test(path.resolve(__dirname, `./build/${buildType}/binding.node`))
.then(() =>
test(path.resolve(__dirname,
`./build/${buildType}/binding_noexcept.node`)));

// Make sure the instance data finalizer is called at process exit. If the hint
// is non-zero, it will be printed out by the child process.
function testFinalizer(bindingName, hint, expected) {
bindingName = bindingName.split('\\').join('\\\\');
const child = spawn(process.execPath, [
'-e',
`require('${bindingName}').addon_data(${hint}).verbose = true;`
]);
const actual = [];
readline
.createInterface({ input: child.stderr })
.on('line', (line) => {
if (expected.indexOf(line) >= 0) {
actual.push(line);
}
})
.on('close', () => assert.deepStrictEqual(expected, actual));
return new Promise((resolve) => {
bindingName = bindingName.split('\\').join('\\\\');
const child = spawn(process.execPath, [
'-e',
`require('${bindingName}').addon_data(${hint}).verbose = true;`
]);
const actual = [];
readline
.createInterface({ input: child.stderr })
.on('line', (line) => {
if (expected.indexOf(line) >= 0) {
actual.push(line);
}
})
.on('close', () => {
assert.deepStrictEqual(expected, actual);
resolve();
});
});
}

function test(bindingName) {
async function test(bindingName) {
const binding = require(bindingName).addon_data(0);

// Make sure it is possible to get/set instance data.
Expand All @@ -37,6 +45,7 @@ function test(bindingName) {
binding.verbose = false;
assert.strictEqual(binding.verbose.verbose, false);

testFinalizer(bindingName, 0, ['addon_data: Addon::~Addon']);
testFinalizer(bindingName, 42, ['addon_data: Addon::~Addon', 'hint: 42']);
await testFinalizer(bindingName, 0, ['addon_data: Addon::~Addon']);
await testFinalizer(bindingName, 42,
['addon_data: Addon::~Addon', 'hint: 42']);
}
28 changes: 9 additions & 19 deletions test/arraybuffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@ const buildType = process.config.target_defaults.default_configuration;
const assert = require('assert');
const testUtil = require('./testUtil');

test(require(`./build/${buildType}/binding.node`));
test(require(`./build/${buildType}/binding_noexcept.node`));
module.exports = test(require(`./build/${buildType}/binding.node`))
.then(() => test(require(`./build/${buildType}/binding_noexcept.node`)));

function test(binding) {
testUtil.runGCTests([
return testUtil.runGCTests([
'Internal ArrayBuffer',
() => {
const test = binding.arraybuffer.createBuffer();
Expand All @@ -25,10 +25,8 @@ function test(binding) {
assert.ok(test instanceof ArrayBuffer);
assert.strictEqual(0, binding.arraybuffer.getFinalizeCount());
},
() => {
global.gc();
assert.strictEqual(0, binding.arraybuffer.getFinalizeCount());
},

() => assert.strictEqual(0, binding.arraybuffer.getFinalizeCount()),

'External ArrayBuffer with finalizer',
() => {
Expand All @@ -37,12 +35,8 @@ function test(binding) {
assert.ok(test instanceof ArrayBuffer);
assert.strictEqual(0, binding.arraybuffer.getFinalizeCount());
},
() => {
global.gc();
},
() => {
assert.strictEqual(1, binding.arraybuffer.getFinalizeCount());
},

() => assert.strictEqual(1, binding.arraybuffer.getFinalizeCount()),

'External ArrayBuffer with finalizer hint',
() => {
Expand All @@ -51,12 +45,8 @@ function test(binding) {
assert.ok(test instanceof ArrayBuffer);
assert.strictEqual(0, binding.arraybuffer.getFinalizeCount());
},
() => {
global.gc();
},
() => {
assert.strictEqual(1, binding.arraybuffer.getFinalizeCount());
},

() => assert.strictEqual(1, binding.arraybuffer.getFinalizeCount()),

'ArrayBuffer with constructor',
() => {
Expand Down
9 changes: 0 additions & 9 deletions test/asyncprogressqueueworker.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,6 @@ class TestWorker : public AsyncProgressQueueWorker<ProgressData> {
worker->Queue();
}

static void CancelWork(const CallbackInfo& info) {
auto wrap = info[0].As<Napi::External<TestWorker>>();
auto worker = wrap.Data();
// We cannot cancel a worker if it got started. So we have to do a quick cancel.
worker->Queue();
worker->Cancel();
}

protected:
void Execute(const ExecutionProgress& progress) override {
using namespace std::chrono_literals;
Expand Down Expand Up @@ -89,7 +81,6 @@ Object InitAsyncProgressQueueWorker(Env env) {
Object exports = Object::New(env);
exports["createWork"] = Function::New(env, TestWorker::CreateWork);
exports["queueWork"] = Function::New(env, TestWorker::QueueWork);
exports["cancelWork"] = Function::New(env, TestWorker::CancelWork);
return exports;
}

Expand Down
81 changes: 33 additions & 48 deletions test/asyncprogressqueueworker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,60 +4,45 @@ const common = require('./common')
const assert = require('assert');
const os = require('os');

test(require(`./build/${buildType}/binding.node`));
test(require(`./build/${buildType}/binding_noexcept.node`));
module.exports = test(require(`./build/${buildType}/binding.node`))
.then(() => test(require(`./build/${buildType}/binding_noexcept.node`)));

function test({ asyncprogressqueueworker }) {
success(asyncprogressqueueworker);
fail(asyncprogressqueueworker);
cancel(asyncprogressqueueworker);
return;
async function test({ asyncprogressqueueworker }) {
await success(asyncprogressqueueworker);
await fail(asyncprogressqueueworker);
}

function success(binding) {
const expected = [0, 1, 2, 3];
const actual = [];
const worker = binding.createWork(expected.length,
common.mustCall((err) => {
if (err) {
assert.fail(err);
}
// All queued items shall be invoked before complete callback.
assert.deepEqual(actual, expected);
}),
common.mustCall((_progress) => {
actual.push(_progress);
}, expected.length)
);
binding.queueWork(worker);
return new Promise((resolve, reject) => {
const expected = [0, 1, 2, 3];
const actual = [];
const worker = binding.createWork(expected.length,
common.mustCall((err) => {
if (err) {
reject(err);
} else {
// All queued items shall be invoked before complete callback.
assert.deepEqual(actual, expected);
resolve();
}
}),
common.mustCall((_progress) => {
actual.push(_progress);
}, expected.length)
);
binding.queueWork(worker);
});
}

function fail(binding) {
const worker = binding.createWork(-1,
common.mustCall((err) => {
assert.throws(() => { throw err }, /test error/)
}),
() => {
assert.fail('unexpected progress report');
}
);
binding.queueWork(worker);
}

function cancel(binding) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to remove this test, because create → queue → cancel is a racy operation. After queueing completes there is a small chance that by the time the next line – cancel – executes, the work will already have been queued. This was happening on

https://ci.nodejs.org/job/node-test-node-addon-api-new/2520/nodes=debian8-64/console

Error: Unknown failure
    at /home/iojs/build/workspace/node-test-node-addon-api-new/nodes/debian8-64/node-addon-api/test/asyncprogressqueueworker.js:67:13
    at new Promise (<anonymous>)
    at cancel (/home/iojs/build/workspace/node-test-node-addon-api-new/nodes/debian8-64/node-addon-api/test/asyncprogressqueueworker.js:52:10)
    at test (/home/iojs/build/workspace/node-test-node-addon-api-new/nodes/debian8-64/node-addon-api/test/asyncprogressqueueworker.js:13:9)
    at async /home/iojs/build/workspace/node-test-node-addon-api-new/nodes/debian8-64/node-addon-api/test/index.js:102:5

We have a similar test in core:

https://github.com/nodejs/node/blob/1f94b89309bcd56a2883cd1b0eb2db610251095e/test/node-api/test_async/test_async.cc#L140-L173

I can only conclude that it works in core because the stack is shallower and therefore the distance between queue and cancel is shorter and so it happens to work.

// make sure the work we are going to cancel will not be
// able to start by using all the threads in the pool.
for (let i = 0; i < os.cpus().length; ++i) {
const worker = binding.createWork(-1, () => {}, () => {});
return new Promise((resolve, reject) => {
const worker = binding.createWork(-1,
common.mustCall((err) => {
assert.throws(() => { throw err }, /test error/);
resolve();
}),
common.mustNotCall()
);
binding.queueWork(worker);
}
const worker = binding.createWork(-1,
() => {
assert.fail('unexpected callback');
},
() => {
assert.fail('unexpected progress report');
}
);
binding.cancelWork(worker);
});
}
61 changes: 32 additions & 29 deletions test/asyncprogressworker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,40 +3,43 @@ const buildType = process.config.target_defaults.default_configuration;
const common = require('./common')
const assert = require('assert');

test(require(`./build/${buildType}/binding.node`));
test(require(`./build/${buildType}/binding_noexcept.node`));
module.exports = test(require(`./build/${buildType}/binding.node`))
.then(() => test(require(`./build/${buildType}/binding_noexcept.node`)));

function test({ asyncprogressworker }) {
success(asyncprogressworker);
fail(asyncprogressworker);
return;
async function test({ asyncprogressworker }) {
await success(asyncprogressworker);
await fail(asyncprogressworker);
}

function success(binding) {
const expected = [0, 1, 2, 3];
const actual = [];
binding.doWork(expected.length,
common.mustCall((err) => {
if (err) {
assert.fail(err);
}
}),
common.mustCall((_progress) => {
actual.push(_progress);
if (actual.length === expected.length) {
assert.deepEqual(actual, expected);
}
}, expected.length)
);
return new Promise((resolve, reject) => {
const expected = [0, 1, 2, 3];
const actual = [];
binding.doWork(expected.length,
common.mustCall((err) => {
if (err) {
reject(err);
}
}),
common.mustCall((_progress) => {
actual.push(_progress);
if (actual.length === expected.length) {
assert.deepEqual(actual, expected);
resolve();
}
}, expected.length)
);
});
}

function fail(binding) {
binding.doWork(-1,
common.mustCall((err) => {
assert.throws(() => { throw err }, /test error/)
}),
() => {
assert.fail('unexpected progress report');
}
);
return new Promise((resolve) => {
binding.doWork(-1,
common.mustCall((err) => {
assert.throws(() => { throw err }, /test error/)
resolve();
}),
common.mustNotCall()
);
});
}
17 changes: 8 additions & 9 deletions test/asyncworker-nocallback.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,13 @@
const buildType = process.config.target_defaults.default_configuration;
const common = require('./common');

test(require(`./build/${buildType}/binding.node`));
test(require(`./build/${buildType}/binding_noexcept.node`));
module.exports = test(require(`./build/${buildType}/binding.node`))
.then(() => test(require(`./build/${buildType}/binding_noexcept.node`)));

function test(binding) {
const resolving = binding.asyncworker.doWorkNoCallback(true, {});
resolving.then(common.mustCall()).catch(common.mustNotCall());
async function test(binding) {
await binding.asyncworker.doWorkNoCallback(true, {})
.then(common.mustCall()).catch(common.mustNotCall());

const rejecting = binding.asyncworker.doWorkNoCallback(false, {});
rejecting.then(common.mustNotCall()).catch(common.mustCall());
return;
}
await binding.asyncworker.doWorkNoCallback(false, {})
.then(common.mustNotCall()).catch(common.mustCall());
}
2 changes: 1 addition & 1 deletion test/asyncworker-persistent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ function test(binding, succeed) {
}));
}

test(binding.persistentasyncworker, false)
module.exports = test(binding.persistentasyncworker, false)
.then(() => test(binding.persistentasyncworker, true))
.then(() => test(noexceptBinding.persistentasyncworker, false))
.then(() => test(noexceptBinding.persistentasyncworker, true));
Loading