From e7c64af4042e8fc5900c44baca2d4484f3b06751 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Wed, 15 Jul 2020 15:45:53 -0700 Subject: [PATCH] n-api: run all finalizers via SetImmediate() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 https://github.com/nodejs/node/issues/34341#issuecomment-658426281, 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 Fixes: https://github.com/nodejs/node/issues/34341 PR-URL: https://github.com/nodejs/node/pull/34386 Reviewed-By: James M Snell Reviewed-By: Anna Henningsen Reviewed-By: Michael Dawson Reviewed-By: Juan José Arboleda --- benchmark/napi/ref/index.js | 10 +- src/js_native_api_v8.cc | 8 +- src/js_native_api_v8.h | 7 + src/node_api.cc | 11 ++ test/common/index.js | 25 ++++ test/js-native-api/7_factory_wrap/test.js | 33 +++-- test/js-native-api/8_passing_wrapped/test.js | 21 +-- test/js-native-api/test_exception/test.js | 11 -- .../test_exception/testFinalizerException.js | 32 ++++ test/js-native-api/test_general/test.js | 52 +++---- .../test_general/testFinalizer.js | 16 +- test/js-native-api/test_reference/test.js | 137 +++++++----------- 12 files changed, 200 insertions(+), 163 deletions(-) create mode 100644 test/js-native-api/test_exception/testFinalizerException.js diff --git a/benchmark/napi/ref/index.js b/benchmark/napi/ref/index.js index 3a5e1988275eaa..a8642a54b21990 100644 --- a/benchmark/napi/ref/index.js +++ b/benchmark/napi/ref/index.js @@ -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)); } diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index daad1907349d63..e99333a6a362d1 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -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 diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 9c737f3c9cc9fc..83e6a0bd02e23c 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -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 last_exception; // We store references in two different lists, depending on whether they have diff --git a/src/node_api.cc b/src/node_api.cc index 314e2c57a0493f..bb203fc03c310f 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -32,6 +32,17 @@ struct node_napi_env__ : public napi_env__ { node_env()->untransferable_object_private_symbol(), v8::True(isolate)); } + + void CallFinalizer(napi_finalize cb, void* data, void* hint) override { + napi_env env = static_cast(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; diff --git a/test/common/index.js b/test/common/index.js index c138c6a8415de0..00341e320c5843 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -662,6 +662,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, @@ -671,6 +695,7 @@ const common = { disableCrashOnUnhandledRejection, expectsError, expectWarning, + gcUntil, getArrayBufferViews, getBufferSources, getCallSite, diff --git a/test/js-native-api/7_factory_wrap/test.js b/test/js-native-api/7_factory_wrap/test.js index 8aaf1b0ba91ee7..ff1516eaa5a092 100644 --- a/test/js-native-api/7_factory_wrap/test.js +++ b/test/js-native-api/7_factory_wrap/test.js @@ -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(); diff --git a/test/js-native-api/8_passing_wrapped/test.js b/test/js-native-api/8_passing_wrapped/test.js index 525993c96d3bad..54c829d9c77dff 100644 --- a/test/js-native-api/8_passing_wrapped/test.js +++ b/test/js-native-api/8_passing_wrapped/test.js @@ -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(); diff --git a/test/js-native-api/test_exception/test.js b/test/js-native-api/test_exception/test.js index 6ec878453f0c22..ff11b702198b88 100644 --- a/test/js-native-api/test_exception/test.js +++ b/test/js-native-api/test_exception/test.js @@ -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'); diff --git a/test/js-native-api/test_exception/testFinalizerException.js b/test/js-native-api/test_exception/testFinalizerException.js new file mode 100644 index 00000000000000..1ba74ba4802a0f --- /dev/null +++ b/test/js-native-api/test_exception/testFinalizerException.js @@ -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); diff --git a/test/js-native-api/test_general/test.js b/test/js-native-api/test_general/test.js index aa3a4eedc56634..de06aecb590529 100644 --- a/test/js-native-api/test_general/test.js +++ b/test/js-native-api/test_general/test.js @@ -51,24 +51,13 @@ 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 = {}; @@ -76,21 +65,32 @@ 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(); diff --git a/test/js-native-api/test_general/testFinalizer.js b/test/js-native-api/test_general/testFinalizer.js index d72a4a44a304d8..54265d61bc37ff 100644 --- a/test/js-native-api/test_general/testFinalizer.js +++ b/test/js-native-api/test_general/testFinalizer.js @@ -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(); diff --git a/test/js-native-api/test_reference/test.js b/test/js-native-api/test_reference/test.js index 389ee11d7e5f5b..0c9d13075f2aa0 100644 --- a/test/js-native-api/test_reference/test.js +++ b/test/js-native-api/test_reference/test.js @@ -1,10 +1,10 @@ 'use strict'; // Flags: --expose-gc -const common = require('../../common'); +const { gcUntil, buildType } = require('../../common'); const assert = require('assert'); -const test_reference = require(`./build/${common.buildType}/test_reference`); +const test_reference = require(`./build/${buildType}/test_reference`); // This test script uses external values with finalizer callbacks // in order to track when values get garbage-collected. Each invocation @@ -13,111 +13,80 @@ assert.strictEqual(test_reference.finalizeCount, 0); // Run each test function in sequence, // with an async delay and GC call between each. -function runTests(i, title, tests) { - if (tests[i]) { - if (typeof tests[i] === 'string') { - title = tests[i]; - runTests(i + 1, title, tests); - } else { - try { - tests[i](); - } catch (e) { - console.error(`Test failed: ${title}`); - throw e; - } - setImmediate(() => { - global.gc(); - runTests(i + 1, title, tests); - }); - } - } -} -runTests(0, undefined, [ - - 'External value without a finalizer', - () => { +async function runTests() { + (() => { const value = test_reference.createExternal(); assert.strictEqual(test_reference.finalizeCount, 0); assert.strictEqual(typeof value, 'object'); test_reference.checkExternal(value); - }, - () => { - assert.strictEqual(test_reference.finalizeCount, 0); - }, + })(); + await gcUntil('External value without a finalizer', + () => (test_reference.finalizeCount === 0)); - 'External value with a finalizer', - () => { + (() => { const value = test_reference.createExternalWithFinalize(); assert.strictEqual(test_reference.finalizeCount, 0); assert.strictEqual(typeof value, 'object'); test_reference.checkExternal(value); - }, - () => { - assert.strictEqual(test_reference.finalizeCount, 1); - }, + })(); + await gcUntil('External value with a finalizer', + () => (test_reference.finalizeCount === 1)); - 'Weak reference', - () => { + (() => { const value = test_reference.createExternalWithFinalize(); assert.strictEqual(test_reference.finalizeCount, 0); test_reference.createReference(value, 0); assert.strictEqual(test_reference.referenceValue, value); - }, - () => { - // 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(); - }, + })(); + // Value should be GC'd because there is only a weak ref + await gcUntil('Weak reference', + () => (test_reference.referenceValue === undefined && + test_reference.finalizeCount === 1)); + test_reference.deleteReference(); - 'Strong reference', - () => { + (() => { const value = test_reference.createExternalWithFinalize(); assert.strictEqual(test_reference.finalizeCount, 0); test_reference.createReference(value, 1); assert.strictEqual(test_reference.referenceValue, value); - }, - () => { - // Value should NOT be GC'd because there is a strong ref - assert.strictEqual(test_reference.finalizeCount, 0); - test_reference.deleteReference(); - }, - () => { - // Value should be GC'd because the strong ref was deleted - assert.strictEqual(test_reference.finalizeCount, 1); - }, + })(); + // Value should NOT be GC'd because there is a strong ref + await gcUntil('Strong reference', + () => (test_reference.finalizeCount === 0)); + test_reference.deleteReference(); + await gcUntil('Strong reference (cont.d)', + () => (test_reference.finalizeCount === 1)); - 'Strong reference, increment then decrement to weak reference', - () => { + (() => { const value = test_reference.createExternalWithFinalize(); assert.strictEqual(test_reference.finalizeCount, 0); test_reference.createReference(value, 1); - }, - () => { - // Value should NOT be GC'd because there is a strong ref - assert.strictEqual(test_reference.finalizeCount, 0); - assert.strictEqual(test_reference.incrementRefcount(), 2); - }, - () => { - // Value should NOT be GC'd because there is a strong ref - assert.strictEqual(test_reference.finalizeCount, 0); - assert.strictEqual(test_reference.decrementRefcount(), 1); - }, - () => { - // Value should NOT be GC'd because there is a strong ref - assert.strictEqual(test_reference.finalizeCount, 0); - assert.strictEqual(test_reference.decrementRefcount(), 0); - }, - () => { - // Value should be GC'd because the ref is now weak! - assert.strictEqual(test_reference.finalizeCount, 1); - test_reference.deleteReference(); - }, - () => { - // Value was already GC'd - assert.strictEqual(test_reference.finalizeCount, 1); - }, -]); + })(); + // Value should NOT be GC'd because there is a strong ref + await gcUntil('Strong reference, increment then decrement to weak reference', + () => (test_reference.finalizeCount === 0)); + assert.strictEqual(test_reference.incrementRefcount(), 2); + // Value should NOT be GC'd because there is a strong ref + await gcUntil( + 'Strong reference, increment then decrement to weak reference (cont.d-1)', + () => (test_reference.finalizeCount === 0)); + assert.strictEqual(test_reference.decrementRefcount(), 1); + // Value should NOT be GC'd because there is a strong ref + await gcUntil( + 'Strong reference, increment then decrement to weak reference (cont.d-2)', + () => (test_reference.finalizeCount === 0)); + assert.strictEqual(test_reference.decrementRefcount(), 0); + // Value should be GC'd because the ref is now weak! + await gcUntil( + 'Strong reference, increment then decrement to weak reference (cont.d-3)', + () => (test_reference.finalizeCount === 1)); + test_reference.deleteReference(); + // Value was already GC'd + await gcUntil( + 'Strong reference, increment then decrement to weak reference (cont.d-4)', + () => (test_reference.finalizeCount === 1)); +} +runTests(); // This test creates a napi_ref on an object that has // been wrapped by napi_wrap and for which the finalizer