GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — errors: improve hideStackFrames by Uzlopak · Pull Request #49990 · nodejs/node

Conversation

@Uzlopak

@Uzlopak Uzlopak commented Oct 1, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gurgunday reacted with rocket emoji bricss and H4ad reacted with eyes emoji
All reactions
  • 🚀 1 reaction
  • 👀 2 reactions
@nodejs-github-bot

nodejs-github-bot commented Oct 1, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/http2
  • @nodejs/net
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Oct 1, 2023
@Uzlopak Uzlopak mentioned this pull request Oct 1, 2023
Node errors are very slow to create nodejs/performance#40
Open
@mcollina

mcollina commented Oct 1, 2023

Copy link
Copy Markdown
Member

what's the benefit/performance diff?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Uzlopak

Uzlopak commented Oct 1, 2023

Copy link
Copy Markdown
Contributor Author

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 reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

benjamingr
benjamingr reviewed Oct 1, 2023
View reviewed changes
Comment thread lib/internal/errors.js Outdated

@benjamingr benjamingr Oct 1, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@Uzlopak Uzlopak Oct 1, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@benjamingr benjamingr Oct 5, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks let me know when ready

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@Uzlopak Uzlopak Oct 8, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this test cover the issue?

https://github.com/nodejs/node/blob/37685b5b2c759035fb0fa551cea2303f9b9661bf/test/parallel/test-errors-hide-stack-frames.js#L32

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Uzlopak commented Oct 2, 2023

Copy link
Copy Markdown
Contributor Author

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.

Qard and benjamingr reacted with rocket emoji
All reactions
  • 🚀 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Uzlopak

Uzlopak commented Oct 2, 2023

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Uzlopak commented Oct 4, 2023

Copy link
Copy Markdown
Contributor Author

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

I would like to discuss this.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@anonrig

anonrig commented Oct 4, 2023

Copy link
Copy Markdown
Member

@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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 4, 2023
@anonrig
anonrig requested review from benjamingr and mcollina October 4, 2023 20:24
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 4, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 4, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tniessen tniessen added the review wanted PRs that need review. label Oct 4, 2023
@github-actions github-actions Bot mentioned this pull request Oct 5, 2023
CI Reliability 2023-10-05 nodejs/reliability#684
Open
13 tasks
@targos

targos commented Oct 5, 2023

Copy link
Copy Markdown
Member

Also @BridgeAR

benjamingr reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Uzlopak
Uzlopak force-pushed the hidestack-frames branch 3 times, most recently from da8ff55 to 134ea2b Compare October 5, 2023 12:05
mcollina
mcollina approved these changes Oct 5, 2023
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Qard
Qard reviewed Oct 5, 2023
View reviewed changes
Comment thread lib/internal/errors.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot mentioned this pull request Oct 6, 2023
CI Reliability 2023-10-06 nodejs/reliability#685
Open
10 tasks
@mcollina

mcollina commented Oct 13, 2023

Copy link
Copy Markdown
Member

You are correct

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Uzlopak
Uzlopak commented Oct 13, 2023
View reviewed changes
Comment thread lib/internal/errors.js
});
if (otherClasses.includes(HideStackFramesError)) {
if (otherClasses.length !== 1) {
otherClasses.forEach((clazz) => {

@Uzlopak Uzlopak Oct 13, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcollina
@anonrig

Should we use a for loop instead?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Uzlopak
Uzlopak commented Oct 13, 2023
View reviewed changes
Comment thread lib/internal/errors.js
otherClasses.forEach((clazz) => {
def[clazz.name] = makeNodeErrorWithCode(clazz, sym);
});
if (otherClasses.includes(HideStackFramesError)) {

@Uzlopak Uzlopak Oct 13, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcollina
@anonrig

Should we use primordials?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@mcollina mcollina Oct 26, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend we do.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Uzlopak

Uzlopak commented Oct 14, 2023

Copy link
Copy Markdown
Contributor Author

@benjamingr
Can you have a look?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

benjamingr
benjamingr reviewed Oct 14, 2023
View reviewed changes

@benjamingr benjamingr left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@benjamingr
benjamingr dismissed their stale review October 14, 2023 20:33

unavailable

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

Uzlopak commented Oct 15, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@anonrig anonrig added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Oct 15, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 15, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 15, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Uzlopak

Uzlopak commented Oct 15, 2023

Copy link
Copy Markdown
Contributor Author

@anonrig
can you retrigger the failed CI,please?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

nodejs-github-bot commented Oct 15, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

nodejs-github-bot commented Oct 23, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mcollina
mcollina approved these changes Nov 2, 2023
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
jasnell
jasnell approved these changes Nov 11, 2023
View reviewed changes
@jasnell jasnell added the commit-queue PRs queued for automated landing through the Commit Queue. label Nov 11, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Nov 11, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 83e6350 into nodejs:main Nov 11, 2023
@nodejs-github-bot

nodejs-github-bot commented Nov 11, 2023

Copy link
Copy Markdown
Collaborator

Landed in 83e6350

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

targos pushed a commit that referenced this pull request Nov 11, 2023
@Uzlopak @targos
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 targos mentioned this pull request Nov 12, 2023
v21.2.0 release proposal #50681
Merged
@Uzlopak
Uzlopak deleted the hidestack-frames branch November 13, 2023 07:42
targos pushed a commit that referenced this pull request Nov 14, 2023
@Uzlopak @targos
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 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
@Uzlopak @richardlau
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 richardlau mentioned this pull request Mar 25, 2024
v20.12.0 proposal #52212
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingr benjamingr benjamingr left review comments
@mcollina mcollina mcollina approved these changes
@jasnell jasnell jasnell approved these changes
@anonrig anonrig anonrig approved these changes
@Qard Qard Awaiting requested review from Qard
@RafaelGSS RafaelGSS Awaiting requested review from RafaelGSS

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. needs-citgm PRs that need a CITGM CI run. review wanted PRs that need review.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

10 participants

@Uzlopak @nodejs-github-bot @mcollina @anonrig @targos @Qard @jasnell @benjamingr @RafaelGSS @tniessen
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.