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:

const err = new Error('foo');
throw err;

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 <info@bnoordhuis.nl> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Feng Yu <F3n67u@outlook.com>

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: #43875 Fixes: #43186 Fixes: #41541 Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Refs: #43875 PR-URL: #44019 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Feng Yu <F3n67u@outlook.com>

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: #43875 Fixes: #43186 Fixes: #41541 Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Refs: #43875 PR-URL: #44019 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Feng Yu <F3n67u@outlook.com>

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

      }
    }
  }
  if (sourceMap && sourceMap.data) {
    return new SourceMap(sourceMap.data);
  }
-  return undefined;
+  return null;


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#43875 Fixes: nodejs#43186 Fixes: nodejs#41541 Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Refs: nodejs#43875 PR-URL: nodejs#44019 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Feng Yu <F3n67u@outlook.com>

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-actions bot 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-actions bot 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 1ef283d Compare July 21, 2022 18:04

github-actions bot 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 d3efe87 Compare 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

legendecas commented Jul 17, 2022 •

edited

Loading

nodejs-github-bot commented Jul 18, 2022

bcoe left a comment

bcoe Jul 19, 2022

legendecas commented Jul 21, 2022

bcoe left a comment

bcoe commented Jul 22, 2022

bcoe commented Jul 22, 2022

mcollina left a comment

mcollina commented Jul 22, 2022

legendecas commented Jul 24, 2022

BridgeAR commented Jul 28, 2022

mcollina commented Jul 28, 2022

danez Aug 25, 2022

mcollina Aug 25, 2022

danez Aug 25, 2022

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

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

Conversation

legendecas commented Jul 17, 2022 • edited Loading

nodejs-github-bot commented Jul 18, 2022

bcoe left a comment

Choose a reason for hiding this comment

bcoe Jul 19, 2022

Choose a reason for hiding this comment

legendecas commented Jul 21, 2022

bcoe left a comment

Choose a reason for hiding this comment

bcoe commented Jul 22, 2022

bcoe commented Jul 22, 2022

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Jul 22, 2022

legendecas commented Jul 24, 2022

BridgeAR commented Jul 28, 2022

mcollina commented Jul 28, 2022

danez Aug 25, 2022

Choose a reason for hiding this comment

mcollina Aug 25, 2022

Choose a reason for hiding this comment

danez Aug 25, 2022

Choose a reason for hiding this comment

legendecas commented Jul 17, 2022 •

edited

Loading