src: fixup Error.stackTraceLimit during snapshot building by joyeecheung · Pull Request #55121 · nodejs/node

joyeecheung · 2024-09-25T18:19:10Z

src: parse --stack-trace-limit and use it in --trace-* flags

Previously, --trace-exit and --trace-sync-io doesn't take care
of --stack-trace-limit and always print a stack trace with maximum
size of 10. This patch parses --stack-trace-limit during
initialization and use the value in --trace-* flags.

src: fixup Error.stackTraceLimit during snapshot building

When V8 creates a context for snapshot building, it does not
install Error.stackTraceLimit. As a result, error.stack would
be undefined in the snapshot builder script unless users
explicitly initialize Error.stackTraceLimit, which may be
surprising.

This patch initializes Error.stackTraceLimit based on the
value of --stack-trace-limit to prevent the surprise. If
users have modified Error.stackTraceLimit in the builder
script, the modified value would be restored during
deserialization. Otherwise, the fixed up limit would be
deleted since V8 expects to find it unset and re-initialize
it during snapshot deserialization.

Fixes: #55100

Previously, --trace-exit and --trace-sync-io doesn't take care of --stack-trace-limit and always print a stack trace with maximum size of 10. This patch parses --stack-trace-limit during initialization and use the value in --trace-* flags.

When V8 creates a context for snapshot building, it does not install Error.stackTraceLimit. As a result, error.stack would be undefined in the snapshot builder script unless users explicitly initialize Error.stackTraceLimit, which may be surprising. This patch initializes Error.stackTraceLimit based on the value of --stack-trace-limit to prevent the surprise. If users have modified Error.stackTraceLimit in the builder script, the modified value would be restored during deserialization. Otherwise, the fixed up limit would be deleted since V8 expects to find it unset and re-initialize it during snapshot deserialization.

nodejs-github-bot · 2024-09-25T18:19:15Z

Review requested:

@nodejs/startup

joyeecheung

I feel that the Error.stackTraceLimit stuff should be documented (and the lack of WebAssembly in the snapshot building context too), but then neither cli.md nor the JS API doc in v8.md feel like the right place to talk about it, so I'll just leave it out and open a separate PR to split a dedicated doc about snapshots and cover them there.

nodejs-github-bot · 2024-09-25T18:35:03Z

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

codecov · 2024-09-25T19:43:53Z

Codecov Report

Attention: Patch coverage is 87.17949% with 10 lines in your changes missing coverage. Please review.

Project coverage is 88.24%. Comparing base (b64006c) to head (ceeee4d).
Report is 406 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/main/mksnapshot.js	87.75%	6 Missing ⚠️
src/node_options.cc	50.00%	2 Missing and 1 partial ⚠️
src/node_options-inl.h	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #55121 +/- ##
=======================================
Coverage 88.24% 88.24% =======================================
Files 652 651 -1 Lines 183912 183960 +48 Branches 35862 35866 +4 =======================================
+ Hits 162296 162342 +46 + Misses 14905 14901 -4 - Partials 6711 6717 +6

Files with missing lines	Coverage Δ
lib/internal/v8/startup_snapshot.js	`99.23% <100.00%> (+0.06%)`	⬆️
src/env-inl.h	`96.56% <100.00%> (+0.01%)`	⬆️
src/env.cc	`85.57% <100.00%> (+0.02%)`	⬆️
src/env.h	`97.95% <ø> (-0.05%)`	⬇️
src/node_options.h	`98.22% <100.00%> (+0.01%)`	⬆️
src/node_options-inl.h	`80.80% <50.00%> (-0.28%)`	⬇️
src/node_options.cc	`87.39% <50.00%> (-0.66%)`	⬇️
lib/internal/main/mksnapshot.js	`95.90% <87.75%> (-1.39%)`	⬇️

... and 59 files with indirect coverage changes

legendecas

LGTM! Since we added addAfterUserSerializeCallback specifically, do we want to cover the case where Error.stackTraceLimit is modified in a user serialize callback?

joyeecheung · 2024-09-26T14:38:27Z

do we want to cover the case where Error.stackTraceLimit is modified in a user serialize callback?

hmm I think this PR already does it, but yeah it's missing a test. Will add one

legendecas · 2024-09-26T14:43:30Z

Yes, I meant a test coverage.

nodejs-github-bot · 2024-09-26T19:52:09Z

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

Co-authored-by: Ashley Claymore <aclaymore@bloomberg.net>

nodejs-github-bot · 2024-09-27T12:05:59Z

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

nodejs-github-bot · 2024-09-27T15:35:05Z

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

nodejs-github-bot · 2024-09-29T17:38:55Z

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

nodejs-github-bot · 2024-09-30T12:56:29Z

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

nodejs-github-bot · 2024-09-30T15:05:14Z

Landed in 371ed85...33bbf37

Previously, --trace-exit and --trace-sync-io doesn't take care of --stack-trace-limit and always print a stack trace with maximum size of 10. This patch parses --stack-trace-limit during initialization and use the value in --trace-* flags. PR-URL: #55121Fixes: #55100 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

When V8 creates a context for snapshot building, it does not install Error.stackTraceLimit. As a result, error.stack would be undefined in the snapshot builder script unless users explicitly initialize Error.stackTraceLimit, which may be surprising. This patch initializes Error.stackTraceLimit based on the value of --stack-trace-limit to prevent the surprise. If users have modified Error.stackTraceLimit in the builder script, the modified value would be restored during deserialization. Otherwise, the fixed up limit would be deleted since V8 expects to find it unset and re-initialize it during snapshot deserialization. PR-URL: #55121Fixes: #55100 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

Previously, --trace-exit and --trace-sync-io doesn't take care of --stack-trace-limit and always print a stack trace with maximum size of 10. This patch parses --stack-trace-limit during initialization and use the value in --trace-* flags. PR-URL: #55121Fixes: #55100 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

When V8 creates a context for snapshot building, it does not install Error.stackTraceLimit. As a result, error.stack would be undefined in the snapshot builder script unless users explicitly initialize Error.stackTraceLimit, which may be surprising. This patch initializes Error.stackTraceLimit based on the value of --stack-trace-limit to prevent the surprise. If users have modified Error.stackTraceLimit in the builder script, the modified value would be restored during deserialization. Otherwise, the fixed up limit would be deleted since V8 expects to find it unset and re-initialize it during snapshot deserialization. PR-URL: #55121Fixes: #55100 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

Previously, --trace-exit and --trace-sync-io doesn't take care of --stack-trace-limit and always print a stack trace with maximum size of 10. This patch parses --stack-trace-limit during initialization and use the value in --trace-* flags. PR-URL: #55121Fixes: #55100 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

When V8 creates a context for snapshot building, it does not install Error.stackTraceLimit. As a result, error.stack would be undefined in the snapshot builder script unless users explicitly initialize Error.stackTraceLimit, which may be surprising. This patch initializes Error.stackTraceLimit based on the value of --stack-trace-limit to prevent the surprise. If users have modified Error.stackTraceLimit in the builder script, the modified value would be restored during deserialization. Otherwise, the fixed up limit would be deleted since V8 expects to find it unset and re-initialize it during snapshot deserialization. PR-URL: #55121Fixes: #55100 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

nodejs/node#55121

* chore: bump node in DEPS to v22.11.0 * src: move evp stuff to ncrypto nodejs/node#54911 * crypto: add Date fields for validTo and validFrom nodejs/node#54159 * module: fix discrepancy between .ts and .js nodejs/node#54461 * esm: do not interpret "main" as a URL nodejs/node#55003 * src: modernize likely/unlikely hints nodejs/node#55155 * chore: update patch indices * crypto: add validFromDate and validToDate fields to X509Certificate nodejs/node#54159 * chore: fixup perfetto patch * fix: clang warning in simdjson * src: add receiver to fast api callback methods nodejs/node#54408 * chore: fixup revert patch * fixup! esm: do not interpret "main" as a URL * fixup! crypto: add Date fields for validTo and validFrom * fix: move ArrayBuffer test patch * src: fixup Error.stackTraceLimit during snapshot building nodejs/node#55121 * fix: bad rebase * chore: fixup amaro * chore: address feedback from review * src: revert filesystem::path changes nodejs/node#55015 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

nodejs/node#55121

* chore: bump node in DEPS to v22.13.0 * chore: bump node in DEPS to v22.13.1 * src: move evp stuff to ncrypto nodejs/node#54911 * crypto: add Date fields for validTo and validFrom nodejs/node#54159 * module: fix discrepancy between .ts and .js nodejs/node#54461 * esm: do not interpret "main" as a URL nodejs/node#55003 * src: modernize likely/unlikely hints nodejs/node#55155 * chore: update patch indices * crypto: add validFromDate and validToDate fields to X509Certificate nodejs/node#54159 * chore: fixup perfetto patch * fix: clang warning in simdjson * src: add receiver to fast api callback methods nodejs/node#54408 * chore: fixup revert patch * fixup! esm: do not interpret "main" as a URL * fixup! crypto: add Date fields for validTo and validFrom * fix: move ArrayBuffer test patch * src: fixup Error.stackTraceLimit during snapshot building nodejs/node#55121 * fix: bad rebase * chore: fixup amaro * chore: address feedback from review * src: revert filesystem::path changes nodejs/node#55015 * chore: fixup GN build file * nodejs/node#55529 * nodejs/node#55798 * nodejs/node#55530 * module: simplify --inspect-brk handling nodejs/node#55679 * src: fix outdated js2c.cc references nodejs/node#56133 * crypto: include openssl/rand.h explicitly nodejs/node#55425 * build: use variable for crypto dep path nodejs/node#55928 * crypto: fix RSA_PKCS1_PADDING error message nodejs/node#55629 * build: use variable for simdutf path nodejs/node#56196 * test,crypto: make crypto tests work with BoringSSL nodejs/node#55491 * fix: suppress clang -Wdeprecated-declarations in libuv libuv/libuv#4486 * deps: update libuv to 1.49.1 nodejs/node#55114 * test: make test-node-output-v8-warning more flexible nodejs/node#55401 * [v22.x] Revert "v8: enable maglev on supported architectures" nodejs/node#54384 * fix: potential WIN32_LEAN_AND_MEAN redefinition c-ares/c-ares#869 * deps: update nghttp2 to 1.64.0 nodejs/node#55559 * src: provide workaround for container-overflow nodejs/node#55591 * build: use variable for simdutf path nodejs/node#56196 * chore: fixup patch indices * fixup! module: simplify --inspect-brk handling * lib: fix fs.readdir recursive async nodejs/node#56041 * lib: avoid excluding symlinks in recursive fs.readdir with filetypes nodejs/node#55714 This doesn't currently play well with ASAR - this should be fixed in a follow up * test: disable CJS permission test for config.main This has diverged as a result of our revert of src,lb: reducing C++ calls of esm legacy main resolve * fixup! lib: fix fs.readdir recursive async * deps: update libuv to 1.49.1 nodejs/node#55114 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

joyeecheung added 2 commits September 25, 2024 20:16

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. 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 Sep 25, 2024

joyeecheung requested a review from legendecas September 25, 2024 18:19

joyeecheung commented Sep 25, 2024
View reviewed changes

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

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2024

targos approved these changes Sep 26, 2024
View reviewed changes

legendecas approved these changes Sep 26, 2024
View reviewed changes

fixup! src: fixup Error.stackTraceLimit during snapshot building
644043c

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

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 26, 2024

acutmore reviewed Sep 26, 2024
View reviewed changes

Comment threadlib/internal/main/mksnapshot.js Outdated
Comment threadlib/internal/main/mksnapshot.js Outdated
Comment threadlib/internal/main/mksnapshot.js Outdated
Comment threadlib/internal/main/mksnapshot.js Outdated

fixup! src: fixup Error.stackTraceLimit during snapshot building
ceeee4d
Co-authored-by: Ashley Claymore <aclaymore@bloomberg.net>

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

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

legendecas approved these changes Sep 27, 2024
View reviewed changes

joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Sep 27, 2024

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

joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Sep 30, 2024

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

nodejs-github-bot closed this Sep 30, 2024

aduh95 mentioned this pull request Oct 9, 2024
V22.10.0 proposal #55343
Merged

marco-ippolito added the dont-land-on-v20.x label Nov 16, 2024

codebytere added a commit to electron/electron that referenced this pull request Nov 22, 2024
src: fixup Error.stackTraceLimit during snapshot building
666967f
nodejs/node#55121

codebytere added a commit to electron/electron that referenced this pull request Dec 3, 2024
src: fixup Error.stackTraceLimit during snapshot building
4b7affd
nodejs/node#55121

codebytere added a commit to electron/electron that referenced this pull request Dec 4, 2024
src: fixup Error.stackTraceLimit during snapshot building
53f47e7
nodejs/node#55121

codebytere added a commit to electron/electron that referenced this pull request Jan 22, 2025
src: fixup Error.stackTraceLimit during snapshot building
6b51859
nodejs/node#55121

codebytere added a commit to electron/electron that referenced this pull request Feb 3, 2025
src: fixup Error.stackTraceLimit during snapshot building
db9f032
nodejs/node#55121

codebytere added a commit to electron/electron that referenced this pull request Feb 10, 2025
src: fixup Error.stackTraceLimit during snapshot building
e7ab6fc
nodejs/node#55121

codebytere added a commit to electron/electron that referenced this pull request Feb 10, 2025
src: fixup Error.stackTraceLimit during snapshot building
3a8825a
nodejs/node#55121

codebytere added a commit to electron/electron that referenced this pull request Feb 14, 2025
src: fixup Error.stackTraceLimit during snapshot building
7fd709a
nodejs/node#55121

Uh oh!

Uh oh!

Conversation

joyeecheung commented Sep 25, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

src: parse --stack-trace-limit and use it in --trace-* flags

src: fixup Error.stackTraceLimit during snapshot building

Uh oh!

nodejs-github-bot commented Sep 25, 2024

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Sep 25, 2024

Uh oh!

codecovBot commented Sep 25, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

legendecas left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Sep 26, 2024

Uh oh!

legendecas commented Sep 26, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 26, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 27, 2024

Uh oh!

nodejs-github-bot commented Sep 27, 2024

Uh oh!

nodejs-github-bot commented Sep 29, 2024

Uh oh!

nodejs-github-bot commented Sep 30, 2024

Uh oh!

nodejs-github-bot commented Sep 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

joyeecheung commented Sep 25, 2024•
edited
Loading

codecovBot commented Sep 25, 2024•
edited
Loading

legendecas commented Sep 26, 2024•
edited
Loading