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

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 539a7ceCompare July 16, 2020 23:23

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

gabrielschulhof commented Jul 17, 2020
View reviewed changes

test/js-native-api/test_exception/test.js Outdated
Copy link
ContributorAuthor
gabrielschulhofJul 17, 2020
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is moved to its own file (testFinalizerException.js), below.

juanarbol approved these changes Jul 17, 2020
View reviewed changes

gabrielschulhof force-pushed the track-down-async-hooks-error branch from fbd5a6b to 17c555aCompare 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-actionsbot 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-actionsbot 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 19793e5Compare July 22, 2020 18:15

github-actionsbot 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 f04eaa7Compare 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 540ec7eCompare July 23, 2020 18:48

github-actionsbot 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

Uh oh!

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

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

Uh oh!

Conversation

gabrielschulhof commented Jul 15, 2020

Checklist

Uh oh!

gabrielschulhof commented Jul 16, 2020

Uh oh!

gabrielschulhof commented Jul 16, 2020

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof commented Jul 16, 2020

Uh oh!

gabrielschulhofJul 17, 2020

Choose a reason for hiding this comment

Uh oh!

Flarna commented Jul 17, 2020

Uh oh!

gabrielschulhof commented Jul 17, 2020

Uh oh!

nodejs-github-bot commented Jul 20, 2020

Uh oh!

nodejs-github-bot commented Jul 20, 2020

Uh oh!

nodejs-github-bot commented Jul 21, 2020

Uh oh!

nodejs-github-bot commented Jul 22, 2020

Uh oh!

gabrielschulhof commented Jul 22, 2020

Uh oh!

gabrielschulhof commented Jul 22, 2020

Uh oh!

nodejs-github-bot commented Jul 23, 2020

Uh oh!

gabrielschulhof commented Jul 23, 2020

Uh oh!

gabrielschulhof commented Jul 23, 2020

Uh oh!

nodejs-github-bot commented Jul 23, 2020

Uh oh!

nodejs-github-bot commented Jul 23, 2020

Uh oh!

nodejs-github-bot commented Jul 23, 2020

Uh oh!

nodejs-github-bot commented Jul 23, 2020

Uh oh!

gabrielschulhof commented Jul 24, 2020

Uh oh!

SimenB commented Aug 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants