errors: improve hideStackFrames #49990

Uzlopak · 2023-10-01T09:36:26Z

This PR improves the hideStackFrames logic.

hideStackFrames is used to hide function calls from stracktraces of Errors. E.g. we call Buffer.alloc(size), then we check if the parameter size is an integer and call validateNumber(size, 'size', 0, kMaxLength);.

'use strict'

try {
    Buffer.alloc('1')
} catch (err) {
    console.error(err.stack)
}

then we get the following stacktrace:

TypeError [ERR_INVALID_ARG_TYPE]: The "size" argument must be of type number. Received type string ('1')
    at Function.alloc (node:buffer:393:3)
    at Object.<anonymous> (/home/aras/workspace/node/test.js:4:12)
    at Module._compile (node:internal/modules/cjs/loader:1369:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1427:10)
    at Module.load (node:internal/modules/cjs/loader:1201:32)
    at Module._load (node:internal/modules/cjs/loader:1017:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:104:12)
    at node:internal/main/run_main_module:23:47

If we remove the hideStackFrame wrapping for demonstration purposes we get:

TypeError [ERR_INVALID_ARG_TYPE]: The "size" argument must be of type number. Received type string ('1')
    at validateNumber (node:internal/validators:177:11)
    at Function.alloc (node:buffer:393:3)
    at Object.<anonymous> (/home/aras/workspace/node/test.js:4:12)
    at Module._compile (node:internal/modules/cjs/loader:1369:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1427:10)
    at Module.load (node:internal/modules/cjs/loader:1201:32)
    at Module._load (node:internal/modules/cjs/loader:1017:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:104:12)
    at node:internal/main/run_main_module:23:47

So we have the additional at validateNumber (node:internal/validators:177:11) entry.

We want to hide this frame, as it is some node internal information, and does not give any benefit for the developer.

Javascript has no inline specifier, so we have to hide it differently. In main branch this is solved by prefixing the function name with __node_internal_ and then generating the whole stacktrace with captureLargerStackTrace, which sets Error.stackTraceLimit = Infinity. When we then call Error.stack, prepareStackTrace will filter out the all frames, which start with __node_internal_.

But we dont need the whole stackTrace. We only need a limited number, by default 10 frames.

To achieve this we just generate the necessary stackTrace.

So hideStackFrames will wrap the original function and will capture the correct stack trace if an error is thrown. Functions which are wrapped have to call the HideStackFramesError Errors and wont have a Stack trace.

An edge case is when a function, which uses hideStackFrames calls another function which is wrapped with hideStackFrames. To avoid double generation of the stack, a function which is wrapped with hideStackFrames exposes a function as attribute of the wrapped function called .withoutStackTrace. The outer function will then get the necessary stack trace.

Usually double wrapped functions are validators. I would have liked to extract them all into validators.js or into module/validators.js. But this would have reduced the readability further.

nodejs-github-bot · 2023-10-01T09:36:30Z

Review requested:

@nodejs/http
@nodejs/http2
@nodejs/net

mcollina · 2023-10-01T09:52:06Z

what's the benefit/performance diff?

Uzlopak · 2023-10-01T10:44:20Z

My benchmarks dont show regressions.
But if we go on, I would ensure that all touched code does not contain any perf regressions.

The benefit is, that the stack trace will be generated propery, without the need to call captureLargerStackTrace. So we wont be forced to keep infinity amount of CallSites just to kick some.

On the long run, i want to try to remove prepareStackTrace.

Currently i investigate what this overrideStackTrace Map is doing, and how we can avoid it.

benjamingr · 2023-10-01T11:54:46Z

lib/internal/errors.js

@@ -372,6 +356,23 @@ function makeSystemErrorWithCode(key) {
  };
 }

+function makeNodeErrorForHideStackFrame(Base) {
+  class HideStackFramesError extends Base {


Won't this mess with the .name of the error and how stack traces look (vs the previous implementation)?

The only reason i created this class, was because to avoid to patch everywhere the logic for Error.stackTraceLimit over and over.

The name should be the same imho. But i can tackle it.

Thanks let me know when ready

Does this test cover the issue?

node/test/parallel/test-errors-hide-stack-frames.js

Line 32 in 37685b5

// Test that validator errors have the correct name and message.

Uzlopak · 2023-10-02T07:58:40Z

The existing tests pass.

Next step is to remove the captureLargerStackTrace function and ensure that the logic is equivalent.

Imho this is an exciting PR for me. If done right, this should improve the performance of basically every error case. And tbh it seems to be a smart solution (patting on my own shoulder).

Also it will cover all the validateX functions.so it should expose less unnecessary stacks.

Uzlopak · 2023-10-02T18:17:56Z

note to myself: Investigate https://docs.google.com/document/d/13Sy_kBIJGP0XT34V1CV3nkWya4TwYx9L3Yv45LdGB6Q/edit#heading=h.9ss45aibqpw2

Uzlopak · 2023-10-04T20:12:17Z

@anonrig can you please add the author-ready tag?

I would like to discuss this.

anonrig · 2023-10-04T20:24:25Z

@anonrig can you please add the author-ready tag?

author-ready requires at least 1 collaborator approval. I don't have enough knowledge to review this. I recommend @benjamingr or @mcollina to leave a review before adding author-ready

nodejs-github-bot · 2023-10-04T20:30:31Z

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

targos · 2023-10-05T06:44:39Z

Also @BridgeAR

mcollina

lgtm

lib/internal/errors.js

mcollina · 2023-10-13T05:47:13Z

You are correct

Uzlopak · 2023-10-13T09:59:01Z

lib/internal/errors.js

-    });
+    if (otherClasses.includes(HideStackFramesError)) {
+      if (otherClasses.length !== 1) {
+        otherClasses.forEach((clazz) => {


@mcollina
@anonrig

Should we use a for loop instead?

Uzlopak · 2023-10-13T09:59:31Z

lib/internal/errors.js

-    otherClasses.forEach((clazz) => {
-      def[clazz.name] = makeNodeErrorWithCode(clazz, sym);
-    });
+    if (otherClasses.includes(HideStackFramesError)) {


@mcollina
@anonrig

Should we use primordials?

I would recommend we do.

Uzlopak · 2023-10-14T02:08:06Z

@benjamingr
Can you have a look?

benjamingr

Don't have capacity, sorry, unblocking so others can progress

unavailable

Uzlopak · 2023-10-15T18:19:44Z

@anonrig
Can you now add the author-ready tag, please?

nodejs-github-bot · 2023-10-15T18:27:53Z

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

Uzlopak · 2023-10-15T22:26:02Z

@anonrig
can you retrigger the failed CI,please?

nodejs-github-bot · 2023-10-15T23:28:57Z

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

nodejs-github-bot · 2023-10-23T16:47:42Z

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

nodejs-github-bot · 2023-11-02T13:07:16Z

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

mcollina

lgtm

nodejs-github-bot · 2023-11-11T16:25:10Z

Landed in 83e6350

PR-URL: #49990 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 Oct 1, 2023

Uzlopak mentioned this pull request Oct 1, 2023

Node errors are very slow to create nodejs/performance#40

Open

benjamingr reviewed Oct 1, 2023

View reviewed changes

Uzlopak force-pushed the hidestack-frames branch from cd3b2c9 to a12adde Compare October 1, 2023 22:39

Uzlopak force-pushed the hidestack-frames branch 4 times, most recently from a2134fc to 3f94be4 Compare October 4, 2023 20:11

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 4, 2023

anonrig requested review from mcollina and benjamingr October 4, 2023 20:24

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 4, 2023

tniessen added the review wanted PRs that need reviews. label Oct 4, 2023

github-actions bot mentioned this pull request Oct 5, 2023

CI Reliability 2023-10-05 nodejs/reliability#684

Open

13 tasks

Uzlopak force-pushed the hidestack-frames branch 3 times, most recently from da8ff55 to 134ea2b Compare October 5, 2023 12:05

mcollina approved these changes Oct 5, 2023

View reviewed changes

Qard reviewed Oct 5, 2023

View reviewed changes

lib/internal/errors.js Outdated Show resolved Hide resolved

github-actions bot mentioned this pull request Oct 6, 2023

CI Reliability 2023-10-06 nodejs/reliability#685

Open

10 tasks

Uzlopak commented Oct 13, 2023

View reviewed changes

benjamingr reviewed Oct 14, 2023

View reviewed changes

Uzlopak requested review from Qard and RafaelGSS October 14, 2023 20:35

anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 15, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 15, 2023

mcollina approved these changes Nov 2, 2023

View reviewed changes

jasnell approved these changes Nov 11, 2023

View reviewed changes

jasnell added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 11, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 11, 2023

nodejs-github-bot merged commit 83e6350 into nodejs:main Nov 11, 2023
57 checks passed

targos pushed a commit that referenced this pull request Nov 11, 2023

errors: improve hideStackFrames

6c3a3c1

PR-URL: #49990 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]>

targos mentioned this pull request Nov 12, 2023

v21.2.0 release proposal #50681

Merged

Uzlopak deleted the hidestack-frames branch November 13, 2023 07:42

targos pushed a commit that referenced this pull request Nov 14, 2023

errors: improve hideStackFrames

14e3675

PR-URL: #49990 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]>

PavelMor25 mentioned this pull request Nov 15, 2023

fix test-server test: incorrect socket hang up processing error in Node v21 DevExpress/testcafe-hammerhead#2978

Merged

richardlau pushed a commit that referenced this pull request Mar 25, 2024

errors: improve hideStackFrames

db7459b

PR-URL: #49990 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]>

richardlau mentioned this pull request Mar 25, 2024

v20.12.0 proposal #52212

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

errors: improve hideStackFrames #49990

errors: improve hideStackFrames #49990

Uzlopak commented Oct 1, 2023 •

edited

Loading

nodejs-github-bot commented Oct 1, 2023

mcollina commented Oct 1, 2023

Uzlopak commented Oct 1, 2023

benjamingr Oct 1, 2023 •

edited

Loading

Uzlopak Oct 1, 2023

benjamingr Oct 5, 2023

Uzlopak Oct 8, 2023

Uzlopak commented Oct 2, 2023

Uzlopak commented Oct 2, 2023

Uzlopak commented Oct 4, 2023

anonrig commented Oct 4, 2023

nodejs-github-bot commented Oct 4, 2023

targos commented Oct 5, 2023

mcollina left a comment

mcollina commented Oct 13, 2023

Uzlopak Oct 13, 2023

Uzlopak Oct 13, 2023

mcollina Oct 26, 2023

Uzlopak commented Oct 14, 2023

benjamingr left a comment

Uzlopak commented Oct 15, 2023 •

edited

Loading

nodejs-github-bot commented Oct 15, 2023

Uzlopak commented Oct 15, 2023

nodejs-github-bot commented Oct 15, 2023

nodejs-github-bot commented Oct 23, 2023

nodejs-github-bot commented Nov 2, 2023 •

edited by jasnell

Loading

mcollina left a comment

nodejs-github-bot commented Nov 11, 2023

errors: improve hideStackFrames #49990

errors: improve hideStackFrames #49990

Conversation

Uzlopak commented Oct 1, 2023 • edited Loading

nodejs-github-bot commented Oct 1, 2023

mcollina commented Oct 1, 2023

Uzlopak commented Oct 1, 2023

benjamingr Oct 1, 2023 • edited Loading

Choose a reason for hiding this comment

Uzlopak Oct 1, 2023

Choose a reason for hiding this comment

benjamingr Oct 5, 2023

Choose a reason for hiding this comment

Uzlopak Oct 8, 2023

Choose a reason for hiding this comment

Uzlopak commented Oct 2, 2023

Uzlopak commented Oct 2, 2023

Uzlopak commented Oct 4, 2023

anonrig commented Oct 4, 2023

nodejs-github-bot commented Oct 4, 2023

targos commented Oct 5, 2023

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Oct 13, 2023

Uzlopak Oct 13, 2023

Choose a reason for hiding this comment

Uzlopak Oct 13, 2023

Choose a reason for hiding this comment

mcollina Oct 26, 2023

Choose a reason for hiding this comment

Uzlopak commented Oct 14, 2023

benjamingr left a comment

Choose a reason for hiding this comment

Uzlopak commented Oct 15, 2023 • edited Loading

nodejs-github-bot commented Oct 15, 2023

Uzlopak commented Oct 15, 2023

nodejs-github-bot commented Oct 15, 2023

nodejs-github-bot commented Oct 23, 2023

nodejs-github-bot commented Nov 2, 2023 • edited by jasnell Loading

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Nov 11, 2023

Uzlopak commented Oct 1, 2023 •

edited

Loading

benjamingr Oct 1, 2023 •

edited

Loading

Uzlopak commented Oct 15, 2023 •

edited

Loading

nodejs-github-bot commented Nov 2, 2023 •

edited by jasnell

Loading