-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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.
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
fix: cjs-module-lexer WebAssembly out of memory fallback #43612
Conversation
Review requested:
|
4de4158
to
700056e
Compare
700056e
to
a522525
Compare
Seems it would fix the issue I was havving: #42222 Thanks a lot @guybedford |
Is there some way to test this? Like some way to test that the JS version of the lexer returns the same results for some set of test cases? And/or test that the JS one actually gets loaded if the wasm init fails? |
The lexer project itself runs test cases against both versions, and we've been using both versions in production for a long time now. In terms of simulating the Wasm memory error I wouldn't know to do that in a safe way on CI. |
You could do it via some external property, like if an environment variable |
Landed in 10ed3e8 |
PR-URL: #43612 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
PR-URL: #43612 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
PR-URL: #43612 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@guybedford, should this fix apply to Docker container node:18.8.0-alpine3.16 on armv7hf builds? If so, it appears the issue still exists. Oddly, it isn't a problem using the exact same deployment on amd64 or arm64 containers to the same service.
|
@Maggie0002 it looks like you're getting Yarn internals in the stack there so perhaps Yarn is running some Wasm? So I'm not sure this is the lexer causing the issue. If it were the lexer, then you would have a lexer line from the internal Node.js stack trace. |
@guybedford, it's across a whole bunch. You think it could be a Yarn thing?
|
PR-URL: nodejs/node#43612 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
PR-URL: #43612 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [node](https://github.com/nodejs/node) | stage | patch | `14.21.1-bullseye-slim` -> `14.21.2-bullseye-slim` | --- ### Release Notes <details> <summary>nodejs/node</summary> ### [`v14.21.2`](https://github.com/nodejs/node/releases/tag/v14.21.2): 2022-12-13, Version 14.21.2 'Fermium' (LTS), @​richardlau [Compare Source](nodejs/node@v14.21.1...v14.21.2) ##### Notable Changes ##### OpenSSL 1.1.1s This update is a bugfix release and does not address any security vulnerabilities. ##### Root certificates updated to NSS 3.85 Certificates added: - Autoridad de Certificacion Firmaprofesional CIF [`A626340`](nodejs/node@A62634068) - Certainly Root E1 - Certainly Root R1 - D-TRUST BR Root CA 1 2020 - D-TRUST EV Root CA 1 2020 - DigiCert TLS ECC P384 Root G5 - DigiCert TLS RSA4096 Root G5 - E-Tugra Global Root CA ECC v3 - E-Tugra Global Root CA RSA v3 - HiPKI Root CA - G1 - ISRG Root X2 - Security Communication ECC RootCA1 - Security Communication RootCA3 - Telia Root CA v2 - vTrus ECC Root CA - vTrus Root CA Certificates removed: - Cybertrust Global Root - DST Root CA X3 - GlobalSign Root CA - R2 - Hellenic Academic and Research Institutions RootCA 2011 ##### Time zone update to 2022f Time zone data has been updated to 2022f. This includes changes to Daylight Savings Time (DST) for Fiji and Mexico. For more information, see <https://mm.icann.org/pipermail/tz-announce/2022-October/000075.html>. ##### Commits - \[[`436a596e99`](nodejs/node@436a596e99)] - **crypto**: update root certificates (Luigi Pinca) [#​45490](nodejs/node#45490) - \[[`4b422d34af`](nodejs/node@4b422d34af)] - **deps**: V8: cherry-pick [`d2db7fa`](nodejs/node@d2db7fa7f786) (Richard Lau) [#​45785](nodejs/node#45785) - \[[`625f4bf3a9`](nodejs/node@625f4bf3a9)] - **deps**: update corepack to 0.15.1 (Node.js GitHub Bot) [#​45331](nodejs/node#45331) - \[[`48a9810de8`](nodejs/node@48a9810de8)] - **deps**: update corepack to 0.15.0 (Node.js GitHub Bot) [#​45235](nodejs/node#45235) - \[[`9f4e64b603`](nodejs/node@9f4e64b603)] - **deps**: update timezone to 2022f (Richard Lau) [#​45521](nodejs/node#45521) - \[[`f297b6bd21`](nodejs/node@f297b6bd21)] - **deps**: update archs files for OpenSSL-1.1.1s (RafaelGSS) [#​45272](nodejs/node#45272) - \[[`11629fef15`](nodejs/node@11629fef15)] - **deps**: upgrade openssl sources to 1.1.1s (RafaelGSS) [#​45272](nodejs/node#45272) - \[[`c3a90c4b44`](nodejs/node@c3a90c4b44)] - **http2**: fix memory leak when nghttp2 hd threshold is reached (rogertyang) [#​41502](nodejs/node#41502) - \[[`785dc3efee`](nodejs/node@785dc3efee)] - **module**: cjs-module-lexer WebAssembly fallback (Guy Bedford) [#​43612](nodejs/node#43612) - \[[`2dbeb889f6`](nodejs/node@2dbeb889f6)] - **node-api**: handle no support for external buffers (Michael Dawson) [#​45181](nodejs/node#45181) - \[[`5b2ea124f3`](nodejs/node@5b2ea124f3)] - **test**: add test to validate changelogs for releases (Richard Lau) [#​45325](nodejs/node#45325) - \[[`f13f889956`](nodejs/node@f13f889956)] - **test**: add a test to ensure the correctness of timezone upgrades (Darshan Sen) [#​45299](nodejs/node#45299) - \[[`5608e6fa72`](nodejs/node@5608e6fa72)] - **tools**: update certdata.txt (Luigi Pinca) [#​45490](nodejs/node#45490) - \[[`d6f1d7107b`](nodejs/node@d6f1d7107b)] - **tools**: have test-asan use ubuntu-20.04 (Filip Skokan) [#​45581](nodejs/node#45581) - \[[`370a00f737`](nodejs/node@370a00f737)] - **tools**: make license-builder.sh comply with shellcheck 0.8.0 (Rich Trott) [#​41258](nodejs/node#41258) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC41NS4wIiwidXBkYXRlZEluVmVyIjoiMzQuNTUuMCJ9--> Reviewed-on: https://git.walbeck.it/mwalbeck/docker-jellyfin-livestream/pulls/210 Co-authored-by: renovate-bot <bot@walbeck.it> Co-committed-by: renovate-bot <bot@walbeck.it>
This implements a possible fix for the Wasm memory issues that have been coming up with hosts that are unable to run WebAssembly in v8 due to low memory requirements.
See the thread here - nodejs/cjs-module-lexer#61, there have also been issues posted in core, some open and some closed.
I don't know why this took me so long, but we can just fallback to the JS lexer implementation if Wasm initialization fails, which this PR should provide.
//cc @nodejs/modules