test: fix unreliable assumption in js-native-api/test_cannot_run_js #51898

joyeecheung · 2024-02-27T15:19:08Z

Previously the test assumes that when the queued finalizer is run, it must be run at a point where env->can_call_into_js() is false (typically, during Environment shutdown), which is not certain. If GC kicks in early and the second pass finalizer is queued before the event loop runs the check callbacks, the finalizer would then be called in check callbacks (via native immediates), where the finalizer can still call into JS. Essentially, addons can't make assumptions about where the queued finalizer would be called. This patch updates the assertions in the test to account for that.

targos

LGTM as a hotfix, but the test will basically become useless once V8 12.2 lands, as it will always return napi_ok then.

Previously the test assumes that when the queued finalizer is run, it must be run at a point where env->can_call_into_js() is false (typically, during Environment shutdown), which is not certain. If GC kicks in early and the second pass finalizer is queued before the event loop runs the check callbacks, the finalizer would then be called in check callbacks (via native immediates), where the finalizer can still call into JS. Essentially, addons can't make assumptions about where the queued finalizer would be called. This patch updates the assertions in the test to account for that.

joyeecheung · 2024-02-27T15:40:13Z

but the test will basically become useless once V8 12.2 lands, as it will always return napi_ok then.

That's not certain, it really depends on what the heap looks like when the addon is loaded. Any subtle change to the internals can delay the GC to happen after the check callbacks are called in the event loop (or not happen at all), and then it'll be napi_cannot_run_js again (and become a flaky nightmare). The fix just makes sure that it works for all cases (no matter GC kicks in before or after check callbacks, or if it doesn't happen at all)

richardlau · 2024-02-27T15:48:17Z

If this fixes the test, perhaps remove the flaky designation in

node/test/root.status

Line 2 in f28ccd3

js-native-api/test_cannot_run_js/test: PASS,FLAKY

?

…un_js

joyeecheung · 2024-02-27T15:52:40Z

If this fixes the test, perhaps remove the flaky designation in

Updated the status file - was the designation even working in the first place? It still got caught in the CI it seems, and locally the test runner still reports it. (I think it should've been marked as PASS,CRASH instead?)

richardlau · 2024-02-27T16:01:12Z

If this fixes the test, perhaps remove the flaky designation in

Updated the status file - was the designation even working in the first place? It still got caught in the CI it seems, and locally the test runner still reports it. (I think it should've been marked as PASS,CRASH instead?)

Yes, it was working -- e.g. https://ci.nodejs.org/job/node-test-commit-ibmi/nodes=ibmi73-ppc64/1466/ (from #48180 (comment)) is yellow due to this test crashing and not red.

nodejs-github-bot · 2024-02-27T16:05:42Z

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

richardlau · 2024-02-28T18:08:12Z

It looks like the addon is failing to compile on Windows:
e.g. https://ci.nodejs.org/job/node-test-binary-windows-native-suites/21625/nodes=win10-vs2019-COMPILED_BY-vs2022/console

17:10:21 Failed to build addon in c:\workspace\node-test-binary-windows-native-suites\node\test\js-native-api\test_cannot_run_js:
17:10:21 Building the projects in this solution one at a time. To enable parallel build, please add the "-m" switch.

17:10:21   test_cannot_run_js.c

17:10:21 c:\workspace\node-test-binary-windows-native-suites\node\test\js-native-api\test_cannot_run_js\test_cannot_run_js.c(28,1): error C2121: '#': invalid character: possibly the result of a macro expansion [c:\workspace\node-test-binary-windows-native-suites\node\test\js-native-api\test_cannot_run_js\build\test_cannot_run_js.vcxproj]

StefanStojanovic · 2024-02-29T10:20:41Z

It looks like the addon is failing to compile on Windows

I tried this locally and can confirm the behavior. What fixed the compilation for me was this. @joyeecheung if you think it's good, feel free to apply it.

diff --git a/test/js-native-api/test_cannot_run_js/test_cannot_run_js.c b/test/js-native-api/test_cannot_run_js/test_cannot_run_js.c
index 925a7117d06..19cc7ae9058 100644
--- a/test/js-native-api/test_cannot_run_js/test_cannot_run_js.c
+++ b/test/js-native-api/test_cannot_run_js/test_cannot_run_js.c
@@ -21,12 +21,13 @@ static void Finalize(napi_env env, void* data, void* hint) {
   // or during environment shutdown (where napi_cannot_run_js or
   // napi_pending_exception is returned). This is not deterministic from
   // the point of view of the addon.
-  NODE_API_NOGC_ASSERT_RETURN_VOID(
 #ifdef NAPI_EXPERIMENTAL
+  NODE_API_NOGC_ASSERT_RETURN_VOID(
       result == napi_cannot_run_js || result == napi_ok,
       "getting named property from global in finalizer should succeed "
       "or return napi_cannot_run_js");
 #else
+  NODE_API_NOGC_ASSERT_RETURN_VOID(
       result == napi_pending_exception || result == napi_ok,
       "getting named property from global in finalizer should succeed "
       "or return napi_pending_exception");

…annot_run_js

nodejs-github-bot · 2024-02-29T16:57:03Z

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

nodejs-github-bot · 2024-02-29T19:13:51Z

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

nodejs-github-bot · 2024-03-01T00:11:24Z

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

nodejs-github-bot · 2024-03-01T01:44:56Z

Landed in 30c9181

Previously the test assumes that when the queued finalizer is run, it must be run at a point where env->can_call_into_js() is false (typically, during Environment shutdown), which is not certain. If GC kicks in early and the second pass finalizer is queued before the event loop runs the check callbacks, the finalizer would then be called in check callbacks (via native immediates), where the finalizer can still call into JS. Essentially, addons can't make assumptions about where the queued finalizer would be called. This patch updates the assertions in the test to account for that. PR-URL: #51898 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

Previously the test assumes that when the queued finalizer is run, it must be run at a point where env->can_call_into_js() is false (typically, during Environment shutdown), which is not certain. If GC kicks in early and the second pass finalizer is queued before the event loop runs the check callbacks, the finalizer would then be called in check callbacks (via native immediates), where the finalizer can still call into JS. Essentially, addons can't make assumptions about where the queued finalizer would be called. This patch updates the assertions in the test to account for that. PR-URL: nodejs#51898 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

Previously the test assumes that when the queued finalizer is run, it must be run at a point where env->can_call_into_js() is false (typically, during Environment shutdown), which is not certain. If GC kicks in early and the second pass finalizer is queued before the event loop runs the check callbacks, the finalizer would then be called in check callbacks (via native immediates), where the finalizer can still call into JS. Essentially, addons can't make assumptions about where the queued finalizer would be called. This patch updates the assertions in the test to account for that. PR-URL: #51898 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

nodejs-github-bot added needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Feb 27, 2024

joyeecheung mentioned this pull request Feb 27, 2024

deps: update V8 to 12.2 #51362

Closed

joyeecheung force-pushed the fix-test-cannot-run branch from 525308f to e3b6d32 Compare February 27, 2024 15:22

targos approved these changes Feb 27, 2024

View reviewed changes

joyeecheung force-pushed the fix-test-cannot-run branch from e3b6d32 to 4165dca Compare February 27, 2024 15:36

fixup! test: fix unreliable assumption in js-native-api/test_cannot_r…

e69bbce

…un_js

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2024

richardlau approved these changes Feb 27, 2024

View reviewed changes

richardlau linked an issue Feb 27, 2024 that may be closed by this pull request

Flaky js-native-api/test_cannot_run_js/test #48180

Closed

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2024

marco-ippolito mentioned this pull request Feb 27, 2024

lib: only build the ESM facade for builtins when they are needed #51669

Closed

marco-ippolito added the v21.x label Feb 27, 2024

marco-ippolito approved these changes Feb 27, 2024

View reviewed changes

legendecas approved these changes Feb 29, 2024

View reviewed changes

fixup! fixup! test: fix unreliable assumption in js-native-api/test_c…

d78f663

…annot_run_js

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 29, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 29, 2024

marco-ippolito mentioned this pull request Feb 29, 2024

Major bug was fixed in NPM@10.3.0 concerning apps started with npm start #51923

Closed

richardlau approved these changes Mar 1, 2024

View reviewed changes

richardlau added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Mar 1, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 1, 2024

nodejs-github-bot merged commit 30c9181 into nodejs:main Mar 1, 2024
57 checks passed

marco-ippolito mentioned this pull request Mar 1, 2024

v21.7.0 proposal #51932

Merged

toyobayashi mentioned this pull request Mar 9, 2024

test: fix unreliable assumption in js-native-api/test_cannot_run_js toyobayashi/emnapi#111

Merged

richardlau mentioned this pull request Mar 25, 2024

v20.12.0 proposal #52212

Merged

richardlau mentioned this pull request Sep 26, 2024

[v18.x backport] test: update TLS tests for OpenSSL 3.2 #55101

Merged

aduh95 mentioned this pull request Nov 7, 2024

2024-11-12, Version 18.20.5 'Hydrogen' (LTS) #55768

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: fix unreliable assumption in js-native-api/test_cannot_run_js #51898

test: fix unreliable assumption in js-native-api/test_cannot_run_js #51898

joyeecheung commented Feb 27, 2024 •

edited

Loading

targos left a comment

joyeecheung commented Feb 27, 2024 •

edited

Loading

richardlau commented Feb 27, 2024

joyeecheung commented Feb 27, 2024

richardlau commented Feb 27, 2024

nodejs-github-bot commented Feb 27, 2024

richardlau commented Feb 28, 2024

StefanStojanovic commented Feb 29, 2024

nodejs-github-bot commented Feb 29, 2024

nodejs-github-bot commented Feb 29, 2024

nodejs-github-bot commented Mar 1, 2024

nodejs-github-bot commented Mar 1, 2024

test: fix unreliable assumption in js-native-api/test_cannot_run_js #51898

test: fix unreliable assumption in js-native-api/test_cannot_run_js #51898

Conversation

joyeecheung commented Feb 27, 2024 • edited Loading

targos left a comment

Choose a reason for hiding this comment

joyeecheung commented Feb 27, 2024 • edited Loading

richardlau commented Feb 27, 2024

joyeecheung commented Feb 27, 2024

richardlau commented Feb 27, 2024

nodejs-github-bot commented Feb 27, 2024

richardlau commented Feb 28, 2024

StefanStojanovic commented Feb 29, 2024

nodejs-github-bot commented Feb 29, 2024

nodejs-github-bot commented Feb 29, 2024

nodejs-github-bot commented Mar 1, 2024

nodejs-github-bot commented Mar 1, 2024

joyeecheung commented Feb 27, 2024 •

edited

Loading

joyeecheung commented Feb 27, 2024 •

edited

Loading