Skip to content

src: do not enable wasm trap handler if there's not enough vmem#62132

Open
joyeecheung wants to merge 3 commits intonodejs:mainfrom
joyeecheung:wasm-handler-2
Open

src: do not enable wasm trap handler if there's not enough vmem#62132
joyeecheung wants to merge 3 commits intonodejs:mainfrom
joyeecheung:wasm-handler-2

Conversation

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Mar 6, 2026

The first commit is a backport of https://chromium-review.googlesource.com/c/v8/v8/+/7638233

This complements --disable-wasm-trap-handler and allows users to keep wasm functional in an environment with virtual memory restrictions (e.g. in the case of VS code server run for remote access, the user does not necessary always have the permission on the server to configure NODE_OPTIONS or --disable-wasm-trap-handler - in fact they don't even necessarily know this has anything to do with WASM on Node.js since they shouldn't be aware of what VS code server is implemented with).

Supersedes #60788 - this keeps the estimation logic in Node.js instead, and also keeps it in POSIX because on other systems the issue that this is solving is not practical (e.g. on Windows, one cannot bound the virtual memory available without bounding the physical memory, and it does not make a difference whether trap based bound checks are enabled or not when physical memory limit is reached)

  1. When the amount of virtual memory available does not allow more than 1 wasm cage at startup, Node.js automatically disables trap-based bound checks for wasm - in this case, no wasm can run at all if we keep the trap-based bound checks, so there's no point enabling the optimization. This should remove a good enough of reproductions of Out of memory: Cannot allocate Wasm memory for new instance (after update from 1.100.3 to 1.101) microsoft/vscode#251777 (which is triggered by the usage of wasm in undici)
  2. When the amount of virtual memory allows more than 1 cage, but still less than the usual 128TB on 64-bit systems, the optimization is still optimistically enabled. In this case some initial wasm cage may be allocated but may fail later when there's not enough virtual memory left. This maintains the current behavior when we unconditionally enable the optimzation.
  3. --disable-wasm-trap-handler is kept as an escape hatch for 2.

We can tweak the threshold of estimation of needed wasm cages in 1 in Node.js to reduce the possibility of 2, this PR currently sets the threshold to 1, since even within Node.js core there's already undici that would use one cage whenever fetch() i sused.

Refs: microsoft/vscode#251777
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/7638233

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/security-wg
  • @nodejs/startup
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 6, 2026
@joyeecheung joyeecheung marked this pull request as draft March 6, 2026 14:51
@codecov
Copy link

codecov bot commented Mar 6, 2026

Codecov Report

❌ Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.67%. Comparing base (c96c3da) to head (c5ded73).
⚠️ Report is 62 commits behind head on main.

Files with missing lines Patch % Lines
src/node.cc 94.44% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #62132      +/-   ##
==========================================
+ Coverage   89.66%   89.67%   +0.01%     
==========================================
  Files         676      676              
  Lines      206550   206710     +160     
  Branches    39546    39579      +33     
==========================================
+ Hits       185198   185366     +168     
- Misses      13478    13500      +22     
+ Partials     7874     7844      -30     
Files with missing lines Coverage Δ
src/debug_utils.h 80.00% <ø> (ø)
src/node.cc 76.78% <94.44%> (+0.39%) ⬆️

... and 83 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

hubot pushed a commit to v8/v8 that referenced this pull request Mar 10, 2026
When the system does not have enough virtual memory for the wasm
cage, installing the trap handler would cause any code allocating
wasm memory to throw. Therefore it's useful for the embedder to
know when the system doesn't have enough virtual address space
to allocate enough wasm cages and in that case, skip the
trap handler installation so that wasm code can at least work
(even not at the maximal performance).

Node.js previously has a command line option
--disable-wasm-trap-handler to fully disable trap-based bound checks,
this new API would allow it to adapt automatically while keeping the
optimization in the happy path, since it's not always possible for
end users to opt-into disabling trap-based bound checks (for example,
when a VS Code Server is loaded in a remote server for debugging).

Refs: nodejs/node#62132
Refs: microsoft/vscode#251777
Change-Id: I345c076af2b2b47700e5716b49c3133fdf8a0981
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7638233
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/main@{#105702}
Original commit message:

    [api] Add V8::GetWasmMemoryReservationSizeInBytes()

    When the system does not have enough virtual memory for the wasm
    cage, installing the trap handler would cause any code allocating
    wasm memory to throw. Therefore it's useful for the embedder to
    know when the system doesn't have enough virtual address space
    to allocate enough wasm cages and in that case, skip the
    trap handler installation so that wasm code can at least work
    (even not at the maximal performance).

    Node.js previously has a command line option
    --disable-wasm-trap-handler to fully disable trap-based bound checks,
    this new API would allow it to adapt automatically while keeping the
    optimization in the happy path, since it's not always possible for
    end users to opt-into disabling trap-based bound checks (for example,
    when a VS Code Server is loaded in a remote server for debugging).

    Refs: nodejs#62132
    Refs: microsoft/vscode#251777
    Change-Id: I345c076af2b2b47700e5716b49c3133fdf8a0981
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7638233
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Reviewed-by: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#105702}

Refs: v8/v8@bef0d9c
Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
joyeecheung added a commit to joyeecheung/node that referenced this pull request Mar 10, 2026
Original commit message:

    [api] Add V8::GetWasmMemoryReservationSizeInBytes()

    When the system does not have enough virtual memory for the wasm
    cage, installing the trap handler would cause any code allocating
    wasm memory to throw. Therefore it's useful for the embedder to
    know when the system doesn't have enough virtual address space
    to allocate enough wasm cages and in that case, skip the
    trap handler installation so that wasm code can at least work
    (even not at the maximal performance).

    Node.js previously has a command line option
    --disable-wasm-trap-handler to fully disable trap-based bound checks,
    this new API would allow it to adapt automatically while keeping the
    optimization in the happy path, since it's not always possible for
    end users to opt-into disabling trap-based bound checks (for example,
    when a VS Code Server is loaded in a remote server for debugging).

    Refs: nodejs#62132
    Refs: microsoft/vscode#251777
    Change-Id: I345c076af2b2b47700e5716b49c3133fdf8a0981
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7638233
    Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Reviewed-by: Clemens Backes <clemensb@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#105702}

Refs: v8/v8@bef0d9c
Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
@nodejs-github-bot

This comment was marked as outdated.

@joyeecheung joyeecheung force-pushed the wasm-handler-2 branch 2 times, most recently from de51d96 to 37b5c46 Compare March 11, 2026 00:22
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung marked this pull request as ready for review March 12, 2026 02:27
@joyeecheung joyeecheung changed the title [WIP] src: do not enable wasm trap handler if there's not enough vmem src: do not enable wasm trap handler if there's not enough vmem Mar 12, 2026
@joyeecheung joyeecheung added the review wanted PRs that need reviews. label Mar 12, 2026
@joyeecheung
Copy link
Member Author

cc @nodejs/cpp-reviewers @nodejs/v8 PTAL, this should address microsoft/vscode#251777 (there are also a bunch of similarly confused users on the Internet affected by this issue, like https://www.reddit.com/r/vscode/comments/1ld6fp7/downloading_vs_code_server_loop_when_connecting/ and https://stackoverflow.com/questions/65055955/vscode-ssh-extension-stuck-on-the-installation-step)

@joyeecheung joyeecheung added wasm Issues and PRs related to WebAssembly. memory Issues and PRs related to the memory management or memory footprint. lts-watch-v22.x PRs that may need to be released in v22.x lts-watch-v24.x PRs that may need to be released in v24.x labels Mar 12, 2026
@Aditi-1400
Copy link
Contributor

Aditi-1400 commented Mar 12, 2026

Also, we should update --disable-wasm-trap-handler documentation in cli.md

@joyeecheung
Copy link
Member Author

@Aditi-1400 Updated the docs, can you take another look? Thanks

test-wasm-allocation: SKIP
test-wasm-allocation-auto-adapt: SKIP
test-wasm-allocation-memory64: SKIP
test-wasm-allocation-memory64-auto-adapt: SKIP
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test file wasn't added, also we should probably add test-wasm-allocation-disable-trap-handler here?

Copy link
Member Author

@joyeecheung joyeecheung Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I merged that test into test-wasm-allocation-auto-adapt; removed, thanks for catching!

As for test-wasm-allocation-disable-trap-handler - there's probably no need to skip, it should pass everywhere regardless (it just allocates a bunch of wasm memory - this can also serve to test that the option is harmless even on platforms where trap handler is not supported in the first place).

@ChALkeR
Copy link
Member

ChALkeR commented Mar 17, 2026

q (non-blocking):

What even is "not enough vmem" on 64-bit systems?
(It's not a real resource it's just address space)

On which setups can this be hit except for manually setting ulimit on vmem? (and e.g. very old OpenVZ bean counting)
What is a valid reason to set ulimit on vmem?
Also ref: #37314

Does this mean that we should start caring about VSZ?
As in, block / make opt-outs from other things that might be using VSZ?

@kxxt
Copy link
Contributor

kxxt commented Mar 21, 2026

q (non-blocking):

What even is "not enough vmem" on 64-bit systems? (It's not a real resource it's just address space)

RISC-V 64bit cpus with sv39 only allocates 256GiB of address space for userspace.

On which setups can this be hit except for manually setting ulimit on vmem? (and e.g. very old OpenVZ bean counting) What is a valid reason to set ulimit on vmem? Also ref: #37314

We are hitting it very often on riscv64 with sv39. e.g. #60591 and riscv-forks/electron#3

It is happening even for an operation simple as yarn install with yarn v3+.

// enabled.
return true;
}
uint64_t virtual_memory_available = static_cast<uint64_t>(lim.rlim_cur);
Copy link
Contributor

@kxxt kxxt Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Non blocking)

The virtual memory available to the process is usually much more than the virtual memory available for usage in wasm trap handler.

e.g. In nodejs/doc-kit#691 (comment) , I observed that only a single wasm memory is successfully allocated, while the available virtual address space is 256GiB (riscv64 with sv39). It's very likely that address space fragmentation caused it.

Maybe it would be better to check if the available virtual memory could contain more cages rather than just one cage. But that being said, I am not sure where to draw the line.

Copy link
Member Author

@joyeecheung joyeecheung Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would help accommodate more use cases yeah, though I guess for the initial version we can just go with RLIMIT_AS for now for the more common "someone is setting it somewhere to be < 16GB" case - which means even core functionality can be incomplete (since llhttp requires at least 1 cage). I think it'll probably be unlikely for 64-bit architectures to have a natural limit that is smaller than that. For space sizes in-between, this is going to be tricky, you'll only see issues from multiple user-land WASM, and that's harder to accomondate just from the Node.js side - you'll likely need something in V8 instead.

@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 22, 2026

What even is "not enough vmem" on 64-bit systems?

From what I tell - likely some legacy operational stuff that sets ulimit -v something (in my case that was why I introduced the --disable-wasm-trap-handler), which the end user is not aware of. This is mostly for the case where the end user doesn't even know they are using Node.js - like you are trying to vscode remoteSSH into a machine, and VSCode drops a code server that's a bundled Node.js application there, and the system happen to have some ulimit setting by the admin. As an end user that doesn't know anything about Node.js, you will just see the remote SSH stuck, and it'll be sufficiently tedious to even find out why it's stuck, or try to convince the admin to unset the limit. (I happened to run into it once after I added the CLI option, and I'd say even though I knew how it works in Node.js/V8, I didn't expect this was why VSCode RemoteSSH was stuck and it was tedious even for me to dig into the plumbing between the two when I just wanted to look at files on the server from VSCode).

As in, block / make opt-outs from other things that might be using VSZ?

I don't think this is common. Most of the time Node.js allocates virtual memory as needed, and don't reserve huge chunk of address space that it doesn't use. Trap-based bound checks in WASM is special in that it reserves a large chunk of address space (currently 8-16GB) even if it just needs to commit 64KB.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lib / src Issues and PRs related to general changes in the lib or src directory. lts-watch-v22.x PRs that may need to be released in v22.x lts-watch-v24.x PRs that may need to be released in v24.x memory Issues and PRs related to the memory management or memory footprint. needs-ci PRs that need a full CI run. review wanted PRs that need reviews. wasm Issues and PRs related to WebAssembly.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants