src,lib: print source map error source on demand#43875

legendecas · 2022-07-17T08:26:40Z

The source context is not prepended to the value of the stack property when the source map is not enabled. Rather than prepending the error source context to the value of the stack property unconditionally, this PR aligns the behavior and only prints the source context when the error is not handled by userland (e.g. fatal errors).

Also, this PR fixes that when source-map support is enabled, the error source context is not pointing to where the error was thrown. For a source file "test.js" like:

consterr=newError('foo');throwerr;

The source context for the thrown error should be:

test.js:2 throw err; // << where the error was thrown ^ Error: foo at test.js:1:13 // << where the error was created

Fixes: #43186
Fixes: #41541

nodejs-github-bot · 2022-07-18T16:44:53Z

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

bcoe

I really like this approach 🎉

One thing I noticed, was the addition of kSourceURLMagicComment, I think this relates to evaluated code? If this addresses an issue we didn't catch with your last PR, perhaps we should add an additional test for it?

bcoe · 2022-07-19T21:25:08Z

src/node_errors.cc

+ Local<Value> argv[] ={script_resource_name,
+v8::Int32::New(isolate, linenum),
+v8::Int32::New(isolate, columnum)};
+ MaybeLocal<Value> maybe_ret = env->get_source_map_error_source()->Call(


I like this approach a lot 👍 good idea moving this behaviour into C++ land.

lib/internal/source_map/prepare_stack_trace.js

lib/internal/source_map/source_map_cache.js

The source context is not prepended to the value of the `stack` property when the source map is not enabled. Rather than prepending the error source context to the value of the `stack` property unconditionally, this patch aligns the behavior and only prints the source context when the error is not handled by userland (e.g. fatal errors). Also, this patch fixes that when source-map support is enabled, the error source context is not pointing to where the error was thrown.

legendecas · 2022-07-21T18:05:01Z

@bcoe thank you for the suggestion! Updated with stricter nullish check. :)

bcoe

LGTM.

If you still have benchmarks setup, mind testing this branch against HEAD, for the source-map benchmarks?

To make sure moving responsiblities into C++ didn't hit performance in any ways.
To see if we actually improved performance in some cases.

bcoe · 2022-07-22T14:27:27Z

@nodejs/cpp-reviewers would be good to have someone take a look at this PR.

bcoe · 2022-07-22T14:31:50Z

@legendecas I'd make the case that, even though you could squint and call this a breaking change, perhaps it's worth landing in a patch, when we weigh the performance benefits against the likeliness of breaking people:

If it breaks people, it's most likely to break test suites.
The benefit of performance improvements for people printing errors might outweigh the cost of breakages for others.

This might be a question worth bringing to @nodejs/tsc

mcollina

LGTM

Have we got a measure of the performance benefit?

I would recommend we land it on Node.js v18 in a minor release but do not backport to Node v16. Having it in Node.js v19 push one year and a few month widespread adoption of this change.

mcollina · 2022-07-22T15:30:25Z

@nodejs/tsc would you be willing to land this in Node v18 with do-not-backport flags for v16?

legendecas · 2022-07-24T16:05:13Z

Updated with a benchmark on accessing the error.stack property and the improvement in my local environment is considerable:

 confidence improvement accuracy (*) (**) (***) es/error-stack.js n=100000 method='sourcemap' *** 59.82 % ±1.97% ±2.65% ±3.50% es/error-stack.js n=100000 method='without-sourcemap' 0.07 % ±0.58% ±0.77% ±1.01%

BridgeAR · 2022-07-28T09:11:16Z

@mcollina any specific reason why it should not be backported to e.g., v16?

mcollina · 2022-07-28T10:44:25Z

Possible breakages. Maybe we could just label it as "backing" and reevaluate in a few months.

Refs: #43875 PR-URL: #44019 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Feng Yu <[email protected]>

The source context is not prepended to the value of the `stack` property when the source map is not enabled. Rather than prepending the error source context to the value of the `stack` property unconditionally, this patch aligns the behavior and only prints the source context when the error is not handled by userland (e.g. fatal errors). Also, this patch fixes that when source-map support is enabled, the error source context is not pointing to where the error was thrown. PR-URL: #43875Fixes: #43186Fixes: #41541 Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Refs: #43875 PR-URL: #44019 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Feng Yu <[email protected]>

Notable changes: bootstrap: implement run-time user-land snapshots via --build-snapshot and --snapshot-blob This patch introduces `--build-snapshot` and `--snapshot-blob` options for creating and using user land snapshots. To generate a snapshot using snapshot.js as an entry point and write the snapshot blob to snapshot.blob: ```bash echo "globalThis.foo = 'I am from the snapshot'" > snapshot.js node --snapshot-blob snapshot.blob --build-snapshot snapshot.js ``` To restore application state from snapshot.blob, with index.js as the entry point script for the deserialized application: ```bash echo "console.log(globalThis.foo)" > index.js node --snapshot-blob snapshot.blob index.js ``` Users can also use the `v8.startupSnapshot` API to specify an entry point at snapshot building time, thus avoiding the need of an additional entry script at deserialization time: ```bash echo "require('v8').startupSnapshot.setDeserializeMainFunction(() => console.log('I am from the snapshot'))" > snapshot.js node --snapshot-blob snapshot.blob --build-snapshot snapshot.js node --snapshot-blob snapshot.blob ``` Contributed by Joyee Cheung in #38905 Other notable changes: * crypto: * (SEMVER-MINOR) allow zero-length IKM in HKDF and in webcrypto PBKDF2 (Filip Skokan) #44201 * (SEMVER-MINOR) allow zero-length secret KeyObject (Filip Skokan) #44201 * doc: * add MoLow to collaborators (Moshe Atlow) #44214 * add ErickWendel to collaborators (Erick Wendel) #44088 * http: * (SEMVER-MINOR) make idle http parser count configurable (theanarkh) #43974 * net: * (SEMVER-MINOR) add local family (theanarkh) #43975 * src: * (SEMVER-MINOR) add detailed embedder process initialization API (Anna Henningsen) #44121 * (SEMVER-MINOR) print source map error source on demand (Chengzhong Wu) #43875 * tls: * (SEMVER-MINOR) pass a valid socket on `tlsClientError` (Daeyeon Jeong) #44021 * worker: * deprecate `--trace-atomics-wait` (Keyhan Vakil) #44093

The source context is not prepended to the value of the `stack` property when the source map is not enabled. Rather than prepending the error source context to the value of the `stack` property unconditionally, this patch aligns the behavior and only prints the source context when the error is not handled by userland (e.g. fatal errors). Also, this patch fixes that when source-map support is enabled, the error source context is not pointing to where the error was thrown. PR-URL: #43875Fixes: #43186Fixes: #41541 Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Refs: #43875 PR-URL: #44019 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Feng Yu <[email protected]>

Notable changes: * bootstrap: * implement run-time user-land snapshots via --build-snapshot and --snapshot-blob (Joyee Cheung) in #38905 * crypto: * (SEMVER-MINOR) allow zero-length IKM in HKDF and in webcrypto PBKDF2 (Filip Skokan) #44201 * (SEMVER-MINOR) allow zero-length secret KeyObject (Filip Skokan) #44201 * deps: * upgrade npm to 8.18.0 (npm team) #44263 - Adds a new npm query cmd * doc: * add Erick Wendel to collaborators (Erick Wendel) #44088 * add MoLow to collaborators (Moshe Atlow) #44214 * deprecate --trace-atomics-wait (Keyhan Vakil) #44093 * http: * (SEMVER-MINOR) make idle http parser count configurable (theanarkh) #43974 * net: * (SEMVER-MINOR) add local family (theanarkh) #43975 * src: * (SEMVER-MINOR) print source map error source on demand (Chengzhong Wu) #43875 * tls: * (SEMVER-MINOR) pass a valid socket on tlsClientError (Daeyeon Jeong) #44021 PR-URL: TBD

Notable changes: * bootstrap: * implement run-time user-land snapshots via --build-snapshot and --snapshot-blob (Joyee Cheung) in #38905 * crypto: * (SEMVER-MINOR) allow zero-length IKM in HKDF and in webcrypto PBKDF2 (Filip Skokan) #44201 * (SEMVER-MINOR) allow zero-length secret KeyObject (Filip Skokan) #44201 * deps: * upgrade npm to 8.18.0 (npm team) #44263 - Adds a new npm query cmd * doc: * add Erick Wendel to collaborators (Erick Wendel) #44088 * add MoLow to collaborators (Moshe Atlow) #44214 * deprecate --trace-atomics-wait (Keyhan Vakil) #44093 * http: * (SEMVER-MINOR) make idle http parser count configurable (theanarkh) #43974 * net: * (SEMVER-MINOR) add local family (theanarkh) #43975 * src: * (SEMVER-MINOR) print source map error source on demand (Chengzhong Wu) #43875 * tls: * (SEMVER-MINOR) pass a valid socket on tlsClientError (Daeyeon Jeong) #44021 PR-URL: #44353

Notable changes: * bootstrap: * implement run-time user-land snapshots via --build-snapshot and --snapshot-blob (Joyee Cheung) in #38905 * crypto: * (SEMVER-MINOR) allow zero-length IKM in HKDF and in webcrypto PBKDF2 (Filip Skokan) #44201 * (SEMVER-MINOR) allow zero-length secret KeyObject (Filip Skokan) #44201 * deps: * upgrade npm to 8.18.0 (npm team) #44263 - Adds a new npm query cmd * doc: * add Erick Wendel to collaborators (Erick Wendel) #44088 * add theanarkh to collaborators (theanarkh) #44131 * add MoLow to collaborators (Moshe Atlow) #44214 * add cola119 to collaborators (cola119) #44248 * deprecate --trace-atomics-wait (Keyhan Vakil) #44093 * http: * (SEMVER-MINOR) make idle http parser count configurable (theanarkh) #43974 * net: * (SEMVER-MINOR) add local family (theanarkh) #43975 * src: * (SEMVER-MINOR) print source map error source on demand (Chengzhong Wu) #43875 * tls: * (SEMVER-MINOR) pass a valid socket on tlsClientError (Daeyeon Jeong) #44021 PR-URL: #44353

danez · 2022-08-25T10:41:56Z

lib/internal/source_map/source_map_cache.js

 returnnewSourceMap(sourceMap.data);
 }
-returnundefined;
+returnnull;


Isn't this a breaking change? AVA for example relies on this that it returns undefined here.

Can you open a separate issue? This looks like something unintended that we can fix.

Notable changes: * bootstrap: * implement run-time user-land snapshots via --build-snapshot and --snapshot-blob (Joyee Cheung) in nodejs#38905 * crypto: * (SEMVER-MINOR) allow zero-length IKM in HKDF and in webcrypto PBKDF2 (Filip Skokan) nodejs#44201 * (SEMVER-MINOR) allow zero-length secret KeyObject (Filip Skokan) nodejs#44201 * deps: * upgrade npm to 8.18.0 (npm team) nodejs#44263 - Adds a new npm query cmd * doc: * add Erick Wendel to collaborators (Erick Wendel) nodejs#44088 * add theanarkh to collaborators (theanarkh) nodejs#44131 * add MoLow to collaborators (Moshe Atlow) nodejs#44214 * add cola119 to collaborators (cola119) nodejs#44248 * deprecate --trace-atomics-wait (Keyhan Vakil) nodejs#44093 * http: * (SEMVER-MINOR) make idle http parser count configurable (theanarkh) nodejs#43974 * net: * (SEMVER-MINOR) add local family (theanarkh) nodejs#43975 * src: * (SEMVER-MINOR) print source map error source on demand (Chengzhong Wu) nodejs#43875 * tls: * (SEMVER-MINOR) pass a valid socket on tlsClientError (Daeyeon Jeong) nodejs#44021 PR-URL: nodejs#44353

The source context is not prepended to the value of the `stack` property when the source map is not enabled. Rather than prepending the error source context to the value of the `stack` property unconditionally, this patch aligns the behavior and only prints the source context when the error is not handled by userland (e.g. fatal errors). Also, this patch fixes that when source-map support is enabled, the error source context is not pointing to where the error was thrown. PR-URL: nodejs#43875Fixes: nodejs#43186Fixes: nodejs#41541 Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Refs: nodejs#43875 PR-URL: nodejs#44019 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Feng Yu <[email protected]>

Notable changes: * bootstrap: * implement run-time user-land snapshots via --build-snapshot and --snapshot-blob (Joyee Cheung) in nodejs#38905 * crypto: * (SEMVER-MINOR) allow zero-length IKM in HKDF and in webcrypto PBKDF2 (Filip Skokan) nodejs#44201 * (SEMVER-MINOR) allow zero-length secret KeyObject (Filip Skokan) nodejs#44201 * deps: * upgrade npm to 8.18.0 (npm team) nodejs#44263 - Adds a new npm query cmd * doc: * add Erick Wendel to collaborators (Erick Wendel) nodejs#44088 * add theanarkh to collaborators (theanarkh) nodejs#44131 * add MoLow to collaborators (Moshe Atlow) nodejs#44214 * add cola119 to collaborators (cola119) nodejs#44248 * deprecate --trace-atomics-wait (Keyhan Vakil) nodejs#44093 * http: * (SEMVER-MINOR) make idle http parser count configurable (theanarkh) nodejs#43974 * net: * (SEMVER-MINOR) add local family (theanarkh) nodejs#43975 * src: * (SEMVER-MINOR) print source map error source on demand (Chengzhong Wu) nodejs#43875 * tls: * (SEMVER-MINOR) pass a valid socket on tlsClientError (Daeyeon Jeong) nodejs#44021 PR-URL: nodejs#44353

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jul 17, 2022

legendecas added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 17, 2022

legendecas marked this pull request as ready for review July 17, 2022 14:50

legendecas requested a review from bcoe July 18, 2022 02:13

bcoe self-assigned this Jul 18, 2022

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 18, 2022

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 18, 2022

This was referenced Jul 19, 2022
CI Reliability 2022-07-19 nodejs/reliability#325
Open
CI Reliability 2022-07-20 nodejs/reliability#326
Open

bcoe suggested changes Jul 20, 2022
View reviewed changes

github-actionsbot mentioned this pull request Jul 21, 2022
CI Reliability 2022-07-21 nodejs/reliability#327
Open
16 tasks

legendecas force-pushed the sourcemap-context branch from 6a97b7f to 1ef283dCompare July 21, 2022 18:04

github-actionsbot mentioned this pull request Jul 22, 2022
CI Reliability 2022-07-22 nodejs/reliability#328
Open
15 tasks

bcoe approved these changes Jul 22, 2022
View reviewed changes

bcoe mentioned this pull request Jul 22, 2022
doc: add performance note to --enable-source-maps docs #43817
Merged

mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Jul 22, 2022

mcollina approved these changes Jul 22, 2022
View reviewed changes

trentm mentioned this pull request Jul 22, 2022
require("elastic-apm-node") influences stacktrace generated with "node --enable-source-maps ..." elastic/apm-agent-nodejs#2833
Closed
3 tasks

This was referenced Jul 23, 2022
CI Reliability 2022-07-23 nodejs/reliability#329
Open
CI Reliability 2022-07-24 nodejs/reliability#330
Open

fixup! src,lib: print source map error source on demand
d3efe87

legendecas force-pushed the sourcemap-context branch from 16aab97 to d3efe87Compare July 24, 2022 16:08

tniessen mentioned this pull request Jul 28, 2022
src: fix bug in GetErrorSource() #44019
Closed

aduh95 added baking-for-lts PRs that need to wait before landing in a LTS release. backport-blocked-v16.x and removed dont-land-on-v16.x labels Jul 28, 2022

danielleadams mentioned this pull request Aug 16, 2022
v18.8.0 release proposal #44246
Closed

ruyadorno mentioned this pull request Aug 23, 2022
v18.8.0 release proposal #44353
Merged

danez reviewed Aug 25, 2022
View reviewed changes

This was referenced Sep 26, 2022
src,lib: print prinstine source when source map source not found #44052
Closed
lib: codify findSourceMap return value when not found #44397
Merged

juanarbol mentioned this pull request Oct 4, 2022
lib: don't match sourceMappingURL in strings #44658
Closed

richardlau mentioned this pull request Aug 24, 2023
Release plan - v16.x Maintenance nodejs/Release#658
Closed

richardlau removed the baking-for-lts PRs that need to wait before landing in a LTS release. label May 8, 2024

JoviDeCroock mentioned this pull request Oct 11, 2024
Performance of stack traces in errors results in high response latency (>1 second) graphql/graphql-js#4028
Closed

Uh oh!

src,lib: print source map error source on demand#43875

src,lib: print source map error source on demand #43875

Uh oh!

Conversation

legendecas commented Jul 17, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 18, 2022

Uh oh!

bcoe left a comment

Choose a reason for hiding this comment

Uh oh!

bcoeJul 19, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

legendecas commented Jul 21, 2022

Uh oh!

bcoe left a comment

Choose a reason for hiding this comment

Uh oh!

bcoe commented Jul 22, 2022

Uh oh!

bcoe commented Jul 22, 2022

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Jul 22, 2022

Uh oh!

legendecas commented Jul 24, 2022

Uh oh!

BridgeAR commented Jul 28, 2022

Uh oh!

mcollina commented Jul 28, 2022

Uh oh!

danezAug 25, 2022

Choose a reason for hiding this comment

Uh oh!

mcollinaAug 25, 2022

Choose a reason for hiding this comment

Uh oh!

danezAug 25, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

legendecas commented Jul 17, 2022•
edited
Loading