Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test-shadow-realm-gc-module can crash on macOS (OOM) #50726

Closed
targos opened this issue Nov 14, 2023 · 15 comments
Closed

test-shadow-realm-gc-module can crash on macOS (OOM) #50726

targos opened this issue Nov 14, 2023 · 15 comments
Labels
macos Issues and PRs related to the macOS platform / OSX. test Issues and PRs related to the tests.

Comments

@targos
Copy link
Member

targos commented Nov 14, 2023 

$ out/Debug/node --experimental-shadow-realm --max-old-space-size=20 /Users/mzasso/git/nodejs/node/test/parallel/test-shadow-realm-gc-module.js

<--- Last few GCs --->

[61276:0x148008000]     3114 ms: Scavenge 17.0 (20.1) -> 16.4 (25.1) MB, 1.33 / 0.00 ms  (average mu = 0.889, current mu = 0.757) allocation failure; 
[61276:0x148008000]     3147 ms: Mark-Compact 20.8 (25.8) -> 18.0 (28.6) MB, 11.12 / 0.00 ms  (+ 0.7 ms in 0 steps since start of marking, biggest step 0.0 ms, walltime since start of marking 39 ms) (average mu = 0.867, current mu = 0.858) allocation fail

<--- JS stacktrace --->

FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory
----- Native stack trace -----

 1: 0x1026d59c8 node::DumpNativeBacktrace(__sFILE*) [/Users/mzasso/git/nodejs/node/out/Debug/node]
 2: 0x102814ef4 node::Abort() [/Users/mzasso/git/nodejs/node/out/Debug/node]
 3: 0x1028151d8 node::ModifyCodeGenerationFromStrings(v8::Local<v8::Context>, v8::Local<v8::Value>, bool) [/Users/mzasso/git/nodejs/node/out/Debug/node]
 4: 0x102b9f8f0 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, v8::OOMDetails const&) [/Users/mzasso/git/nodejs/node/out/Debug/node]
 5: 0x1030362f8 v8::internal::Heap::stack() [/Users/mzasso/git/nodejs/node/out/Debug/node]
 6: 0x1030341d4 v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) [/Users/mzasso/git/nodejs/node/out/Debug/node]
...
79: 0x1042501c4 main [/Users/mzasso/git/nodejs/node/out/Debug/node]
80: 0x18157d0e0 start [/usr/lib/dyld]
[1]    61276 abort      out/Debug/node --experimental-shadow-realm --max-old-space-size=20
@targos targos added test Issues and PRs related to the tests. macos Issues and PRs related to the macOS platform / OSX. labels Nov 14, 2023
@MrJithil
Copy link
Member 

node:internal/process/task_queues:95
    runMicrotasks();
    ^

TypeError: Cannot import in ShadowRealm (TypeError: A dynamic import callback was not specified.)

I ran on macOS locally, above is the error message I got with Release/node

@targos targos changed the title test-shadow-realm-gc-module crashes on macOS in debug build test-shadow-realm-gc-module can crash on macOS (OOM) Nov 14, 2023
@targos
Copy link
Member Author

targos commented Nov 14, 2023

I just tried with the release build and I get the same OOM error. I updated the title.

@targos
Copy link
Member Author

targos commented Nov 14, 2023

/cc @legendecas who added the test in #48655

@legendecas
Copy link
Member

legendecas commented Nov 14, 2023 •
edited
Loading

I checked the last two GitHub action runs on the main branch and I didn't find this failure on the macOS build (https://github.com/nodejs/node/actions/runs/6859065494/job/18650800024 failed for a different test). And I didn't reproduce the failure with 1000 runs locally. Does this problem constantly reproduce with your local build?

Stress test: https://ci.nodejs.org/job/node-stress-single-test/464

@targos
Copy link
Member Author

targos commented Nov 14, 2023

I am not close to my computer now but it reproduced consistently (I tried to run it between 5 and 10 times )

@MrJithil
Copy link
Member

With the latest merge? I took the latest and now unable to reproduce

for i in {1..100}; do ./node --experimental-shadow-realm --max-old-space-size=20 test/parallel/test-shadow-realm-gc-module.js; done

@targos
Copy link
Member Author

targos commented Nov 14, 2023

I have a arm64 Mac. Maybe it's platform specific

@MrJithil
Copy link
Member

I am using M1 Pro chip. Is there any customisations you did like node max heap size?

@targos
Copy link
Member Author

targos commented Nov 14, 2023

tools/test.py parallel/test-shadow-realm-gc-module --repeat 100
[00:10|% 100|+ 1|- 99]: Done

I am using M1 Pro chip. Is there any customisations you did like node max heap size?

I always build with ./configure --ninja --node-builtin-modules-path=$(pwd) in case that's relevant.

@MrJithil
Copy link
Member

Let me check.

@marco-ippolito
Copy link
Member

marco-ippolito commented Nov 14, 2023 •
edited
Loading

I've seen this failure in CI on different platforms, https://ci.nodejs.org/job/node-test-commit-linuxone/nodes=rhel8-s390x/40888/console

@targos targos mentioned this issue Nov 14, 2023
Closed
@legendecas
Copy link
Member

I can confirm this can be reproduced with the configure flag --node-builtin-modules-path, in either the release build or the debug build.

@joyeecheung
Copy link
Member

joyeecheung commented Nov 14, 2023 •
edited
Loading

I think it's probably caused by the limit (20MB) being a bit too small for --node-builtin-modules-path which reads the JS sources from disks and creates new strings for every realm, instead of what we normally do (using externalized strings shared across the realms). And then the test is running the allocation in a hot loop so the GC cannot keep up. Awaiting a promisified setTimeout in the loop to give the GC some breathing room fixes it locally for me.

@joyeecheung joyeecheung mentioned this issue Nov 14, 2023
Closed
@joyeecheung
Copy link
Member

joyeecheung commented Nov 17, 2023 •
edited
Loading

I am now seeing test-shadow-realm-gc flakes in V8's Node.js integration test CI too (in different unrelated CLs). We should at least try to get #50735 landed quickly. If that's still not enough to fix it we should mark it so to avoid spreading this flake to V8.

joyeecheung added a commit that referenced this issue Nov 17, 2023
@joyeecheung
test: give more time to GC in test-shadow-realm-gc-*
c5eb2c4 
When --node-builtin-modules-path is used, we read and create
new strings for builtins in each realm which increases the memory
usage. As a result GC may not be able to keep up with the
allocation done in the loop in the test.

As a workaround, give GC a bit more time by waiting for a timer
in the loop.

PR-URL: #50735
Refs: #50726
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
targos pushed a commit that referenced this issue Nov 23, 2023
@joyeecheung @targos
test: give more time to GC in test-shadow-realm-gc-*
91dbe13 
When --node-builtin-modules-path is used, we read and create
new strings for builtins in each realm which increases the memory
usage. As a result GC may not be able to keep up with the
allocation done in the loop in the test.

As a workaround, give GC a bit more time by waiting for a timer
in the loop.

PR-URL: #50735
Refs: #50726
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
martenrichter pushed a commit to martenrichter/node that referenced this issue Nov 26, 2023
@joyeecheung @martenrichter
test: give more time to GC in test-shadow-realm-gc-*
62e045c 
When --node-builtin-modules-path is used, we read and create
new strings for builtins in each realm which increases the memory
usage. As a result GC may not be able to keep up with the
allocation done in the loop in the test.

As a workaround, give GC a bit more time by waiting for a timer
in the loop.

PR-URL: nodejs#50735
Refs: nodejs#50726
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
lucshi pushed a commit to lucshi/node that referenced this issue Nov 27, 2023
@joyeecheung
test: give more time to GC in test-shadow-realm-gc-*
377c28d 
When --node-builtin-modules-path is used, we read and create
new strings for builtins in each realm which increases the memory
usage. As a result GC may not be able to keep up with the
allocation done in the loop in the test.

As a workaround, give GC a bit more time by waiting for a timer
in the loop.

PR-URL: nodejs#50735
Refs: nodejs#50726
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@targos
Copy link
Member Author

targos commented Nov 28, 2023

Can't reproduce anymore. We can reopen if it happens again.

@targos targos closed this as completed Nov 28, 2023
RafaelGSS pushed a commit that referenced this issue Nov 29, 2023
@joyeecheung @RafaelGSS
test: give more time to GC in test-shadow-realm-gc-*
6475cee 
When --node-builtin-modules-path is used, we read and create
new strings for builtins in each realm which increases the memory
usage. As a result GC may not be able to keep up with the
allocation done in the loop in the test.

As a workaround, give GC a bit more time by waiting for a timer
in the loop.

PR-URL: #50735
Refs: #50726
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
RafaelGSS pushed a commit that referenced this issue Nov 30, 2023
@joyeecheung @RafaelGSS
test: give more time to GC in test-shadow-realm-gc-*
136df08 
When --node-builtin-modules-path is used, we read and create
new strings for builtins in each realm which increases the memory
usage. As a result GC may not be able to keep up with the
allocation done in the loop in the test.

As a workaround, give GC a bit more time by waiting for a timer
in the loop.

PR-URL: #50735
Refs: #50726
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
richardlau pushed a commit that referenced this issue Mar 25, 2024
@joyeecheung @richardlau
test: give more time to GC in test-shadow-realm-gc-*
26a06b0 
When --node-builtin-modules-path is used, we read and create
new strings for builtins in each realm which increases the memory
usage. As a result GC may not be able to keep up with the
allocation done in the loop in the test.

As a workaround, give GC a bit more time by waiting for a timer
in the loop.

PR-URL: #50735
Refs: #50726
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
macos Issues and PRs related to the macOS platform / OSX. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@targos @joyeecheung @legendecas @MrJithil @marco-ippolito