n-api: run all finalizers via SetImmediate() #34386

gabrielschulhof · 2020-07-15T23:32:38Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

gabrielschulhof · 2020-07-16T01:35:01Z

@addaleax this changes the gc-tests like test/js-native-api/test_reference significantly, because it involves another iteration of the event loop.

gabrielschulhof · 2020-07-16T16:17:04Z

For testing the finalizers we need to move from a model where you

set up the test,
run the gc,
check for expected values

to one where you

set up the test,
keep running the gc until the expected values appear (with a timeout after which you give up and declare the test failed)

mhdawson

LGTM

gabrielschulhof · 2020-07-16T23:26:48Z

@addaleax @mhdawson thanks for reviewing! I moved the test from test_reference to test_exception which already had a similar test, and I updated all other tests involving gc to await the changes expected in response to the gc instead of asserting that they happened. This means that the tests will time out and fail if the effects of the gc are not as expected instead of failing at an assertion.

PTAL!

gabrielschulhof · 2020-07-17T05:18:25Z

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

-  // To assuage the linter's concerns.
-  (function() {})(x);
-}
-testFinalize('createExternal');


This test is moved to its own file (testFinalizerException.js), below.

Flarna · 2020-07-17T13:30:32Z

Could you please add a little more info in commit message which async hooks error this fixes?

gabrielschulhof · 2020-07-17T22:06:14Z

@Flarna I squashed the commits and elaborated in the commit message on the error that is being fixed.

nodejs-github-bot · 2020-07-20T15:07:36Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32412/

nodejs-github-bot · 2020-07-20T17:00:10Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32424/

nodejs-github-bot · 2020-07-21T19:20:12Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32452/

nodejs-github-bot · 2020-07-22T00:22:29Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32463/

gabrielschulhof · 2020-07-22T06:34:45Z

Oh, boy! Looks like I'd better drop that second commit and make it a separate PR 😳

gabrielschulhof · 2020-07-22T18:16:52Z

Re-wrote the napi/ref benchmark to be async because it was running synchronously and so the SetImmediates never had a chance to delete the refs.

nodejs-github-bot · 2020-07-23T16:56:46Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32484/

gabrielschulhof · 2020-07-23T16:57:47Z

Fixed the linting.

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 nodejs#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 <[email protected]> Fixes: nodejs#34341

gabrielschulhof · 2020-07-23T18:12:38Z

Used assert.match() to get a better idea as to why the test is failing.

nodejs-github-bot · 2020-07-23T18:13:52Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32485/

nodejs-github-bot · 2020-07-23T18:41:31Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32486/

nodejs-github-bot · 2020-07-23T18:49:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32487/

nodejs-github-bot · 2020-07-23T21:41:03Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32495/

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 <[email protected]> Fixes: #34341 PR-URL: #34386 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>

gabrielschulhof · 2020-07-24T06:29:04Z

Landed in a74a6e3.

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 <[email protected]> Fixes: #34341 PR-URL: #34386 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>

SimenB · 2020-08-05T15:40:01Z

Is this the cause of #34636?

Refs: nodejs/node#34636 Refs: nodejs/node#34386

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 <[email protected]> Fixes: #34341 PR-URL: #34386 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>

gabrielschulhof requested review from addaleax and mhdawson July 15, 2020 23:32

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. labels Jul 15, 2020

addaleax approved these changes Jul 15, 2020

View reviewed changes

mhdawson approved these changes Jul 16, 2020

View reviewed changes

gabrielschulhof force-pushed the track-down-async-hooks-error branch from 35f55e0 to 539a7ce Compare July 16, 2020 23:23

gabrielschulhof force-pushed the track-down-async-hooks-error branch from 539a7ce to 738e298 Compare July 16, 2020 23:30

gabrielschulhof commented Jul 17, 2020

View reviewed changes

juanarbol approved these changes Jul 17, 2020

View reviewed changes

gabrielschulhof force-pushed the track-down-async-hooks-error branch from fbd5a6b to 17c555a Compare July 17, 2020 22:05

gabrielschulhof changed the title ~~Track down async hooks error~~ n-api: run all finalizers via SetImmediate() Jul 18, 2020

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 20, 2020

github-actions bot mentioned this pull request Jul 21, 2020

CI Relability 2020-07-21 JaneaSystems/nodejs-reliability#44

Open

10 tasks

jasnell approved these changes Jul 21, 2020

View reviewed changes

github-actions bot mentioned this pull request Jul 22, 2020

CI Relability 2020-07-22 JaneaSystems/nodejs-reliability#45

Open

5 tasks

gabrielschulhof force-pushed the track-down-async-hooks-error branch 2 times, most recently from 17c555a to 19793e5 Compare July 22, 2020 18:15

github-actions bot mentioned this pull request Jul 23, 2020

CI Relability 2020-07-23 JaneaSystems/nodejs-reliability#46

Open

6 tasks

gabrielschulhof force-pushed the track-down-async-hooks-error branch from a1c9d78 to f04eaa7 Compare July 23, 2020 18:12

fixup! testFinalizerException to gc until it throws

540ec7e

gabrielschulhof force-pushed the track-down-async-hooks-error branch from cb4d479 to 540ec7e Compare July 23, 2020 18:48

github-actions bot mentioned this pull request Jul 24, 2020

CI Relability 2020-07-24 JaneaSystems/nodejs-reliability#47

Open

6 tasks

gabrielschulhof closed this Jul 24, 2020

gabrielschulhof deleted the track-down-async-hooks-error branch July 24, 2020 06:29

This was referenced Jul 25, 2020

CI Relability 2020-07-25 JaneaSystems/nodejs-reliability#48

Open

CI Relability 2020-07-26 JaneaSystems/nodejs-reliability#49

Open

CI Relability 2020-07-27 JaneaSystems/nodejs-reliability#50

Open

ruyadorno mentioned this pull request Jul 28, 2020

v14.7.0 proposal #34542

Merged

gabrielschulhof mentioned this pull request Jul 30, 2020

Make sure wrapcallback is used nodejs/node-addon-api#762

Closed

SimenB mentioned this pull request Aug 5, 2020

weak-napi broken in Node 14.7.0 (working in 14.6.0) #34636

Closed

addaleax added a commit to node-ffi-napi/weak-napi that referenced this pull request Aug 5, 2020

Adjust tests for timing differences on Node 14.7+

fdafbde

Refs: nodejs/node#34636 Refs: nodejs/node#34386

addaleax added a commit to node-ffi-napi/ref-napi that referenced this pull request Aug 5, 2020

Adjust tests for timing differences on Node 14.7+

0b6bbb3

Refs: nodejs/node#34636 Refs: nodejs/node#34386

codebytere mentioned this pull request Sep 28, 2020

v12.19.0 proposal #35401

Merged

mhdawson mentioned this pull request Jan 18, 2022

pummel/test-worker-take-heapsnapshot - crash in debug mode on arm #41204

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

n-api: run all finalizers via SetImmediate() #34386

n-api: run all finalizers via SetImmediate() #34386

gabrielschulhof commented Jul 15, 2020

gabrielschulhof commented Jul 16, 2020

gabrielschulhof commented Jul 16, 2020

mhdawson left a comment

gabrielschulhof commented Jul 16, 2020

gabrielschulhof Jul 17, 2020

Flarna commented Jul 17, 2020

gabrielschulhof commented Jul 17, 2020

nodejs-github-bot commented Jul 20, 2020

nodejs-github-bot commented Jul 20, 2020

nodejs-github-bot commented Jul 21, 2020

nodejs-github-bot commented Jul 22, 2020

gabrielschulhof commented Jul 22, 2020

gabrielschulhof commented Jul 22, 2020

nodejs-github-bot commented Jul 23, 2020

gabrielschulhof commented Jul 23, 2020

gabrielschulhof commented Jul 23, 2020

nodejs-github-bot commented Jul 23, 2020

nodejs-github-bot commented Jul 23, 2020

nodejs-github-bot commented Jul 23, 2020

nodejs-github-bot commented Jul 23, 2020

gabrielschulhof commented Jul 24, 2020

SimenB commented Aug 5, 2020

n-api: run all finalizers via SetImmediate() #34386

n-api: run all finalizers via SetImmediate() #34386

Conversation

gabrielschulhof commented Jul 15, 2020

Checklist

gabrielschulhof commented Jul 16, 2020

gabrielschulhof commented Jul 16, 2020

mhdawson left a comment

Choose a reason for hiding this comment

gabrielschulhof commented Jul 16, 2020

gabrielschulhof Jul 17, 2020

Choose a reason for hiding this comment

Flarna commented Jul 17, 2020

gabrielschulhof commented Jul 17, 2020

nodejs-github-bot commented Jul 20, 2020

nodejs-github-bot commented Jul 20, 2020

nodejs-github-bot commented Jul 21, 2020

nodejs-github-bot commented Jul 22, 2020

gabrielschulhof commented Jul 22, 2020

gabrielschulhof commented Jul 22, 2020

nodejs-github-bot commented Jul 23, 2020

gabrielschulhof commented Jul 23, 2020

gabrielschulhof commented Jul 23, 2020

nodejs-github-bot commented Jul 23, 2020

nodejs-github-bot commented Jul 23, 2020

nodejs-github-bot commented Jul 23, 2020

nodejs-github-bot commented Jul 23, 2020

gabrielschulhof commented Jul 24, 2020

SimenB commented Aug 5, 2020