Skip to content

Commit

Permalink
n-api: run all finalizers via SetImmediate()
Browse files Browse the repository at this point in the history
Throwing an exception from a finalizer can cause the following fatal
error:

Error: async hook stack has become corrupted (actual: 2, expected: 0)
 1: 0x970b5a node::InternalCallbackScope::~InternalCallbackScope()
    [./node]
 2: 0x99dda0 node::Environment::RunTimers(uv_timer_s*) [./node]
 3: 0x13d8b22  [./node]
 4: 0x13dbe42 uv_run [./node]
 5: 0xa57974 node::NodeMainInstance::Run() [./node]
 6: 0x9dbc17 node::Start(int, char**) [./node]
 7: 0x7f4965417f43 __libc_start_main [/lib64/libc.so.6]
 8: 0x96f4ae _start [./node]

By #34341 (comment),
calling into JS from a finalizer and/or throwing exceptions from there
is not advised, because the stack may or may not be set up for JS
execution. The best solution is to run the user's finalizer from a
`SetImmediate()` callback.

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Fixes: #34341
PR-URL: #34386
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
  • Loading branch information
Gabriel Schulhof authored and addaleax committed Sep 22, 2020
1 parent afd6e46 commit ba2e341
Show file tree
Hide file tree
Showing 12 changed files with 200 additions and 163 deletions.
10 changes: 6 additions & 4 deletions benchmark/napi/ref/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@ function callNewWeak() {
function main({ n }) {
addon.count = 0;
bench.start();
while (addon.count < n) {
callNewWeak();
}
bench.end(n);
new Promise((resolve) => {
(function oneIteration() {
callNewWeak();
setImmediate(() => ((addon.count < n) ? oneIteration() : resolve()));
})();
}).then(() => bench.end(n));
}
8 changes: 1 addition & 7 deletions src/js_native_api_v8.cc
Original file line number Diff line number Diff line change
Expand Up @@ -267,13 +267,7 @@ class RefBase : protected Finalizer, RefTracker {
protected:
inline void Finalize(bool is_env_teardown = false) override {
if (_finalize_callback != nullptr) {
v8::HandleScope handle_scope(_env->isolate);
_env->CallIntoModule([&](napi_env env) {
_finalize_callback(
env,
_finalize_data,
_finalize_hint);
});
_env->CallFinalizer(_finalize_callback, _finalize_data, _finalize_hint);
}

// this is safe because if a request to delete the reference
Expand Down
7 changes: 7 additions & 0 deletions src/js_native_api_v8.h
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,13 @@ struct napi_env__ {
}
}

virtual void CallFinalizer(napi_finalize cb, void* data, void* hint) {
v8::HandleScope handle_scope(isolate);
CallIntoModule([&](napi_env env) {
cb(env, data, hint);
});
}

v8impl::Persistent<v8::Value> last_exception;

// We store references in two different lists, depending on whether they have
Expand Down
11 changes: 11 additions & 0 deletions src/node_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,17 @@ struct node_napi_env__ : public napi_env__ {
node_env()->arraybuffer_untransferable_private_symbol(),
v8::True(isolate));
}

void CallFinalizer(napi_finalize cb, void* data, void* hint) override {
napi_env env = static_cast<napi_env>(this);
node_env()->SetImmediate([=](node::Environment* node_env) {
v8::HandleScope handle_scope(env->isolate);
v8::Context::Scope context_scope(env->context());
env->CallIntoModule([&](napi_env env) {
cb(env, data, hint);
});
});
}
};

typedef node_napi_env__* node_napi_env;
Expand Down
25 changes: 25 additions & 0 deletions test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -675,6 +675,30 @@ function skipIfDumbTerminal() {
}
}

function gcUntil(name, condition) {
if (typeof name === 'function') {
condition = name;
name = undefined;
}
return new Promise((resolve, reject) => {
let count = 0;
function gcAndCheck() {
setImmediate(() => {
count++;
global.gc();
if (condition()) {
resolve();
} else if (count < 10) {
gcAndCheck();
} else {
reject(name === undefined ? undefined : 'Test ' + name + ' failed');
}
});
}
gcAndCheck();
});
}

const common = {
allowGlobals,
buildType,
Expand All @@ -685,6 +709,7 @@ const common = {
expectsError,
expectsInternalAssertion,
expectWarning,
gcUntil,
getArrayBufferViews,
getBufferSources,
getCallSite,
Expand Down
33 changes: 17 additions & 16 deletions test/js-native-api/7_factory_wrap/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,21 @@ const assert = require('assert');
const test = require(`./build/${common.buildType}/binding`);

assert.strictEqual(test.finalizeCount, 0);
(() => {
const obj = test.createObject(10);
assert.strictEqual(obj.plusOne(), 11);
assert.strictEqual(obj.plusOne(), 12);
assert.strictEqual(obj.plusOne(), 13);
})();
global.gc();
assert.strictEqual(test.finalizeCount, 1);
async function runGCTests() {
(() => {
const obj = test.createObject(10);
assert.strictEqual(obj.plusOne(), 11);
assert.strictEqual(obj.plusOne(), 12);
assert.strictEqual(obj.plusOne(), 13);
})();
await common.gcUntil('test 1', () => (test.finalizeCount === 1));

(() => {
const obj2 = test.createObject(20);
assert.strictEqual(obj2.plusOne(), 21);
assert.strictEqual(obj2.plusOne(), 22);
assert.strictEqual(obj2.plusOne(), 23);
})();
global.gc();
assert.strictEqual(test.finalizeCount, 2);
(() => {
const obj2 = test.createObject(20);
assert.strictEqual(obj2.plusOne(), 21);
assert.strictEqual(obj2.plusOne(), 22);
assert.strictEqual(obj2.plusOne(), 23);
})();
await common.gcUntil('test 2', () => (test.finalizeCount === 2));
}
runGCTests();
21 changes: 12 additions & 9 deletions test/js-native-api/8_passing_wrapped/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,16 @@ const common = require('../../common');
const assert = require('assert');
const addon = require(`./build/${common.buildType}/binding`);

let obj1 = addon.createObject(10);
let obj2 = addon.createObject(20);
const result = addon.add(obj1, obj2);
assert.strictEqual(result, 30);
async function runTest() {
let obj1 = addon.createObject(10);
let obj2 = addon.createObject(20);
const result = addon.add(obj1, obj2);
assert.strictEqual(result, 30);

// Make sure the native destructor gets called.
obj1 = null;
obj2 = null;
global.gc();
assert.strictEqual(addon.finalizeCount(), 2);
// Make sure the native destructor gets called.
obj1 = null;
obj2 = null;
await common.gcUntil('8_passing_wrapped',
() => (addon.finalizeCount() === 2));
}
runTest();
11 changes: 0 additions & 11 deletions test/js-native-api/test_exception/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,14 +66,3 @@ const test_exception = (function() {
'Exception state did not remain clear as expected,' +
` .wasPending() returned ${exception_pending}`);
}

// Make sure that exceptions that occur during finalization are propagated.
function testFinalize(binding) {
let x = test_exception[binding]();
x = null;
assert.throws(() => { global.gc(); }, /Error during Finalize/);

// To assuage the linter's concerns.
(function() {})(x);
}
testFinalize('createExternal');
32 changes: 32 additions & 0 deletions test/js-native-api/test_exception/testFinalizerException.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
'use strict';
if (process.argv[2] === 'child') {
const common = require('../../common');
// Trying, catching the exception, and finding the bindings at the `Error`'s
// `binding` property is done intentionally, because we're also testing what
// happens when the add-on entry point throws. See test.js.
try {
require(`./build/${common.buildType}/test_exception`);
} catch (anException) {
anException.binding.createExternal();
}

// Collect garbage 10 times. At least one of those should throw the exception
// and cause the whole process to bail with it, its text printed to stderr and
// asserted by the parent process to match expectations.
let gcCount = 10;
(function gcLoop() {
global.gc();
if (--gcCount > 0) {
setImmediate(() => gcLoop());
}
})();
return;
}

const assert = require('assert');
const { spawnSync } = require('child_process');
const child = spawnSync(process.execPath, [
'--expose-gc', __filename, 'child'
]);
assert.strictEqual(child.signal, null);
assert.match(child.stderr.toString(), /Error during Finalize/m);
52 changes: 26 additions & 26 deletions test/js-native-api/test_general/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,46 +51,46 @@ assert.strictEqual(test_general.testGetVersion(), 6);
// for null
assert.strictEqual(test_general.testNapiTypeof(null), 'null');

// Ensure that garbage collecting an object with a wrapped native item results
// in the finalize callback being called.
let w = {};
test_general.wrap(w);
w = null;
global.gc();
const derefItemWasCalled = test_general.derefItemWasCalled();
assert.strictEqual(derefItemWasCalled, true,
'deref_item() was called upon garbage collecting a ' +
'wrapped object. test_general.derefItemWasCalled() ' +
`returned ${derefItemWasCalled}`);


// Assert that wrapping twice fails.
const x = {};
test_general.wrap(x);
assert.throws(() => test_general.wrap(x),
{ name: 'Error', message: 'Invalid argument' });
// Clean up here, otherwise derefItemWasCalled() will be polluted.
test_general.removeWrap(x);

// Ensure that wrapping, removing the wrap, and then wrapping again works.
const y = {};
test_general.wrap(y);
test_general.removeWrap(y);
// Wrapping twice succeeds if a remove_wrap() separates the instances
test_general.wrap(y);

// Ensure that removing a wrap and garbage collecting does not fire the
// finalize callback.
let z = {};
test_general.testFinalizeWrap(z);
test_general.removeWrap(z);
z = null;
global.gc();
const finalizeWasCalled = test_general.finalizeWasCalled();
assert.strictEqual(finalizeWasCalled, false,
'finalize callback was not called upon garbage collection.' +
' test_general.finalizeWasCalled() ' +
`returned ${finalizeWasCalled}`);
// Clean up here, otherwise derefItemWasCalled() will be polluted.
test_general.removeWrap(y);

// Test napi_adjust_external_memory
const adjustedValue = test_general.testAdjustExternalMemory();
assert.strictEqual(typeof adjustedValue, 'number');
assert(adjustedValue > 0);

async function runGCTests() {
// Ensure that garbage collecting an object with a wrapped native item results
// in the finalize callback being called.
assert.strictEqual(test_general.derefItemWasCalled(), false);

(() => test_general.wrap({}))();
await common.gcUntil('deref_item() was called upon garbage collecting a ' +
'wrapped object.',
() => test_general.derefItemWasCalled());

// Ensure that removing a wrap and garbage collecting does not fire the
// finalize callback.
let z = {};
test_general.testFinalizeWrap(z);
test_general.removeWrap(z);
z = null;
await common.gcUntil(
'finalize callback was not called upon garbage collection.',
() => (!test_general.finalizeWasCalled()));
}
runGCTests();
16 changes: 10 additions & 6 deletions test/js-native-api/test_general/testFinalizer.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,13 @@ global.gc();

// Add an item to an object that is already wrapped, and ensure that its
// finalizer as well as the wrap finalizer gets called.
let finalizeAndWrap = {};
test_general.wrap(finalizeAndWrap);
test_general.addFinalizerOnly(finalizeAndWrap, common.mustCall());
finalizeAndWrap = null;
global.gc();
assert.strictEqual(test_general.derefItemWasCalled(), true);
async function testFinalizeAndWrap() {
assert.strictEqual(test_general.derefItemWasCalled(), false);
let finalizeAndWrap = {};
test_general.wrap(finalizeAndWrap);
test_general.addFinalizerOnly(finalizeAndWrap, common.mustCall());
finalizeAndWrap = null;
await common.gcUntil('test finalize and wrap',
() => test_general.derefItemWasCalled());
}
testFinalizeAndWrap();
Loading

0 comments on commit ba2e341

Please sign in to comment.