Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit ba2e341

Browse filesBrowse files
Gabriel Schulhofaddaleax
Gabriel Schulhof
authored and
addaleax
committed
n-api: run all finalizers via SetImmediate()
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>
1 parent afd6e46 commit ba2e341
Copy full SHA for ba2e341

File tree

Expand file treeCollapse file tree

12 files changed

+200
-163
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

12 files changed

+200
-163
lines changed
Open diff view settings
Collapse file

‎benchmark/napi/ref/index.js‎

Copy file name to clipboardExpand all lines: benchmark/napi/ref/index.js
+6-4Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,10 @@ function callNewWeak() {
1010
function main({ n }) {
1111
addon.count = 0;
1212
bench.start();
13-
while (addon.count < n) {
14-
callNewWeak();
15-
}
16-
bench.end(n);
13+
new Promise((resolve) => {
14+
(function oneIteration() {
15+
callNewWeak();
16+
setImmediate(() => ((addon.count < n) ? oneIteration() : resolve()));
17+
})();
18+
}).then(() => bench.end(n));
1719
}
Collapse file

‎src/js_native_api_v8.cc‎

Copy file name to clipboardExpand all lines: src/js_native_api_v8.cc
+1-7Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -267,13 +267,7 @@ class RefBase : protected Finalizer, RefTracker {
267267
protected:
268268
inline void Finalize(bool is_env_teardown = false) override {
269269
if (_finalize_callback != nullptr) {
270-
v8::HandleScope handle_scope(_env->isolate);
271-
_env->CallIntoModule([&](napi_env env) {
272-
_finalize_callback(
273-
env,
274-
_finalize_data,
275-
_finalize_hint);
276-
});
270+
_env->CallFinalizer(_finalize_callback, _finalize_data, _finalize_hint);
277271
}
278272

279273
// this is safe because if a request to delete the reference
Collapse file

‎src/js_native_api_v8.h‎

Copy file name to clipboardExpand all lines: src/js_native_api_v8.h
+7Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,13 @@ struct napi_env__ {
101101
}
102102
}
103103

104+
virtual void CallFinalizer(napi_finalize cb, void* data, void* hint) {
105+
v8::HandleScope handle_scope(isolate);
106+
CallIntoModule([&](napi_env env) {
107+
cb(env, data, hint);
108+
});
109+
}
110+
104111
v8impl::Persistent<v8::Value> last_exception;
105112

106113
// We store references in two different lists, depending on whether they have
Collapse file

‎src/node_api.cc‎

Copy file name to clipboardExpand all lines: src/node_api.cc
+11Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,17 @@ struct node_napi_env__ : public napi_env__ {
3232
node_env()->arraybuffer_untransferable_private_symbol(),
3333
v8::True(isolate));
3434
}
35+
36+
void CallFinalizer(napi_finalize cb, void* data, void* hint) override {
37+
napi_env env = static_cast<napi_env>(this);
38+
node_env()->SetImmediate([=](node::Environment* node_env) {
39+
v8::HandleScope handle_scope(env->isolate);
40+
v8::Context::Scope context_scope(env->context());
41+
env->CallIntoModule([&](napi_env env) {
42+
cb(env, data, hint);
43+
});
44+
});
45+
}
3546
};
3647

3748
typedef node_napi_env__* node_napi_env;
Collapse file

‎test/common/index.js‎

Copy file name to clipboardExpand all lines: test/common/index.js
+25Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -675,6 +675,30 @@ function skipIfDumbTerminal() {
675675
}
676676
}
677677

678+
function gcUntil(name, condition) {
679+
if (typeof name === 'function') {
680+
condition = name;
681+
name = undefined;
682+
}
683+
return new Promise((resolve, reject) => {
684+
let count = 0;
685+
function gcAndCheck() {
686+
setImmediate(() => {
687+
count++;
688+
global.gc();
689+
if (condition()) {
690+
resolve();
691+
} else if (count < 10) {
692+
gcAndCheck();
693+
} else {
694+
reject(name === undefined ? undefined : 'Test ' + name + ' failed');
695+
}
696+
});
697+
}
698+
gcAndCheck();
699+
});
700+
}
701+
678702
const common = {
679703
allowGlobals,
680704
buildType,
@@ -685,6 +709,7 @@ const common = {
685709
expectsError,
686710
expectsInternalAssertion,
687711
expectWarning,
712+
gcUntil,
688713
getArrayBufferViews,
689714
getBufferSources,
690715
getCallSite,
Collapse file

‎test/js-native-api/7_factory_wrap/test.js‎

Copy file name to clipboardExpand all lines: test/js-native-api/7_factory_wrap/test.js
+17-16Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -6,20 +6,21 @@ const assert = require('assert');
66
const test = require(`./build/${common.buildType}/binding`);
77

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

18-
(() => {
19-
const obj2 = test.createObject(20);
20-
assert.strictEqual(obj2.plusOne(), 21);
21-
assert.strictEqual(obj2.plusOne(), 22);
22-
assert.strictEqual(obj2.plusOne(), 23);
23-
})();
24-
global.gc();
25-
assert.strictEqual(test.finalizeCount, 2);
18+
(() => {
19+
const obj2 = test.createObject(20);
20+
assert.strictEqual(obj2.plusOne(), 21);
21+
assert.strictEqual(obj2.plusOne(), 22);
22+
assert.strictEqual(obj2.plusOne(), 23);
23+
})();
24+
await common.gcUntil('test 2', () => (test.finalizeCount === 2));
25+
}
26+
runGCTests();
Collapse file

‎test/js-native-api/8_passing_wrapped/test.js‎

Copy file name to clipboardExpand all lines: test/js-native-api/8_passing_wrapped/test.js
+12-9Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,16 @@ const common = require('../../common');
55
const assert = require('assert');
66
const addon = require(`./build/${common.buildType}/binding`);
77

8-
let obj1 = addon.createObject(10);
9-
let obj2 = addon.createObject(20);
10-
const result = addon.add(obj1, obj2);
11-
assert.strictEqual(result, 30);
8+
async function runTest() {
9+
let obj1 = addon.createObject(10);
10+
let obj2 = addon.createObject(20);
11+
const result = addon.add(obj1, obj2);
12+
assert.strictEqual(result, 30);
1213

13-
// Make sure the native destructor gets called.
14-
obj1 = null;
15-
obj2 = null;
16-
global.gc();
17-
assert.strictEqual(addon.finalizeCount(), 2);
14+
// Make sure the native destructor gets called.
15+
obj1 = null;
16+
obj2 = null;
17+
await common.gcUntil('8_passing_wrapped',
18+
() => (addon.finalizeCount() === 2));
19+
}
20+
runTest();
Collapse file

‎test/js-native-api/test_exception/test.js‎

Copy file name to clipboardExpand all lines: test/js-native-api/test_exception/test.js
-11Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -66,14 +66,3 @@ const test_exception = (function() {
6666
'Exception state did not remain clear as expected,' +
6767
` .wasPending() returned ${exception_pending}`);
6868
}
69-
70-
// Make sure that exceptions that occur during finalization are propagated.
71-
function testFinalize(binding) {
72-
let x = test_exception[binding]();
73-
x = null;
74-
assert.throws(() => { global.gc(); }, /Error during Finalize/);
75-
76-
// To assuage the linter's concerns.
77-
(function() {})(x);
78-
}
79-
testFinalize('createExternal');
Collapse file

‎test/js-native-api/test_exception/testFinalizerException.js‎

Copy file name to clipboard
+32Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
'use strict';
2+
if (process.argv[2] === 'child') {
3+
const common = require('../../common');
4+
// Trying, catching the exception, and finding the bindings at the `Error`'s
5+
// `binding` property is done intentionally, because we're also testing what
6+
// happens when the add-on entry point throws. See test.js.
7+
try {
8+
require(`./build/${common.buildType}/test_exception`);
9+
} catch (anException) {
10+
anException.binding.createExternal();
11+
}
12+
13+
// Collect garbage 10 times. At least one of those should throw the exception
14+
// and cause the whole process to bail with it, its text printed to stderr and
15+
// asserted by the parent process to match expectations.
16+
let gcCount = 10;
17+
(function gcLoop() {
18+
global.gc();
19+
if (--gcCount > 0) {
20+
setImmediate(() => gcLoop());
21+
}
22+
})();
23+
return;
24+
}
25+
26+
const assert = require('assert');
27+
const { spawnSync } = require('child_process');
28+
const child = spawnSync(process.execPath, [
29+
'--expose-gc', __filename, 'child'
30+
]);
31+
assert.strictEqual(child.signal, null);
32+
assert.match(child.stderr.toString(), /Error during Finalize/m);
Collapse file

‎test/js-native-api/test_general/test.js‎

Copy file name to clipboardExpand all lines: test/js-native-api/test_general/test.js
+26-26Lines changed: 26 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -51,46 +51,46 @@ assert.strictEqual(test_general.testGetVersion(), 6);
5151
// for null
5252
assert.strictEqual(test_general.testNapiTypeof(null), 'null');
5353

54-
// Ensure that garbage collecting an object with a wrapped native item results
55-
// in the finalize callback being called.
56-
let w = {};
57-
test_general.wrap(w);
58-
w = null;
59-
global.gc();
60-
const derefItemWasCalled = test_general.derefItemWasCalled();
61-
assert.strictEqual(derefItemWasCalled, true,
62-
'deref_item() was called upon garbage collecting a ' +
63-
'wrapped object. test_general.derefItemWasCalled() ' +
64-
`returned ${derefItemWasCalled}`);
65-
66-
6754
// Assert that wrapping twice fails.
6855
const x = {};
6956
test_general.wrap(x);
7057
assert.throws(() => test_general.wrap(x),
7158
{ name: 'Error', message: 'Invalid argument' });
59+
// Clean up here, otherwise derefItemWasCalled() will be polluted.
60+
test_general.removeWrap(x);
7261

7362
// Ensure that wrapping, removing the wrap, and then wrapping again works.
7463
const y = {};
7564
test_general.wrap(y);
7665
test_general.removeWrap(y);
7766
// Wrapping twice succeeds if a remove_wrap() separates the instances
7867
test_general.wrap(y);
79-
80-
// Ensure that removing a wrap and garbage collecting does not fire the
81-
// finalize callback.
82-
let z = {};
83-
test_general.testFinalizeWrap(z);
84-
test_general.removeWrap(z);
85-
z = null;
86-
global.gc();
87-
const finalizeWasCalled = test_general.finalizeWasCalled();
88-
assert.strictEqual(finalizeWasCalled, false,
89-
'finalize callback was not called upon garbage collection.' +
90-
' test_general.finalizeWasCalled() ' +
91-
`returned ${finalizeWasCalled}`);
68+
// Clean up here, otherwise derefItemWasCalled() will be polluted.
69+
test_general.removeWrap(y);
9270

9371
// Test napi_adjust_external_memory
9472
const adjustedValue = test_general.testAdjustExternalMemory();
9573
assert.strictEqual(typeof adjustedValue, 'number');
9674
assert(adjustedValue > 0);
75+
76+
async function runGCTests() {
77+
// Ensure that garbage collecting an object with a wrapped native item results
78+
// in the finalize callback being called.
79+
assert.strictEqual(test_general.derefItemWasCalled(), false);
80+
81+
(() => test_general.wrap({}))();
82+
await common.gcUntil('deref_item() was called upon garbage collecting a ' +
83+
'wrapped object.',
84+
() => test_general.derefItemWasCalled());
85+
86+
// Ensure that removing a wrap and garbage collecting does not fire the
87+
// finalize callback.
88+
let z = {};
89+
test_general.testFinalizeWrap(z);
90+
test_general.removeWrap(z);
91+
z = null;
92+
await common.gcUntil(
93+
'finalize callback was not called upon garbage collection.',
94+
() => (!test_general.finalizeWasCalled()));
95+
}
96+
runGCTests();

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.