Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

errors: fix stacktrace of SystemError, add more tests and benchmark #49956

Closed
wants to merge 1 commit into from

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Sep 29, 2023

Before I wanted to tackle the performance of SystemError I wanted better test coverage for SystemError.

While I was working on the tests, I realized that the stacktrace for SystemError is wrong. I fixed it accordingly ;). It also has slightly performance gains.

So my proposal is:

  • Review and merge of this PR.
  • Follow Up PR for optimizing the SystemError instanciation

Benefit of this separate PR is, that we can see by the changes in the tests, if the behaviour of SystemError got changed.

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. needs-ci PRs that need a full CI run. labels Sep 29, 2023
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 29, 2023

@anonrig
@mcollina
@nodejs/performance

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 30, 2023

Is this a candidate for fast-track?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2023
@nodejs-github-bot

This comment was marked as outdated.

@benjamingr
Copy link
Member

Is this a candidate for fast-track?

No, I'm not sure it should land as is since we (at least I) don't fully understand the implications. Ideally folks who wrote the original code or contributed to it could weigh in and say "yes, this was why"

@benjamingr
Copy link
Member

Generally when code looks "weird" in Node it either fixes a bug/edge case (though I'd expect a test in this case we're pretty diligent about making PRs include tests) or it's there for some precondition that is no longer true.

Maybe @BridgeAR knows? or @devsnek ?

@benjamingr benjamingr added the blocked PRs that are blocked by other issues or PRs. label Sep 30, 2023
@nodejs-github-bot

This comment was marked as outdated.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 30, 2023

Just that it does not get overseen because it is a review remark:

'use strict'

const { join } = require('path');
const { cp } = require('fs');
const net = require('net');

const sock = join(__dirname, `${process.pid}.sock`);
const server = net.createServer();
server.listen(sock);

cp(sock, __dirname, (err) => {
    console.log(err.stack)
    server.close();
});

before (main):

aras@aras-Lenovo-Legion-5-17ARH05H:~/workspace/node$ node test.js 
SystemError [ERR_FS_CP_NON_DIR_TO_DIR]: Cannot overwrite non-directory with directory: cp returned ENOTDIR (cannot overwrite non-directory /home/aras/workspace/node/51128.sock with directory /home/aras/workspace/node) /home/aras/workspace/node
    at new SystemError (node:internal/errors:256:5)
    at new NodeError (node:internal/errors:367:7)
    at checkPaths (node:internal/fs/cp/cp:98:13)
    at async cpFn (node:internal/fs/cp/cp:65:17)

after (this PR):

aras@aras-Lenovo-Legion-5-17ARH05H:~/workspace/node$ ./node test.js 
SystemError [ERR_FS_CP_NON_DIR_TO_DIR]: Cannot overwrite non-directory with directory: cp returned ENOTDIR (cannot overwrite non-directory /home/aras/workspace/node/51188.sock with directory /home/aras/workspace/node) /home/aras/workspace/node
    at checkPaths (node:internal/fs/cp/cp:98:13)
    at async cpFn (node:internal/fs/cp/cp:65:17)

@devsnek
Copy link
Member

devsnek commented Sep 30, 2023

stackTraceLimit is definitely still in use here. I think the bug is that we should be calling Error.captureStackTrace(this, new.target) instead of Error.captureStackTrace(this). Passing new.target as the 2nd parameter will hide the two extra stack frames you are seeing for NodeError and SystemError.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 30, 2023

Actually... in captureLargerStackTrace

ErrorCaptureStackTrace(err);

you need to change it to

ErrorCaptureStackTrace(err, err.constructor);

But I wonder if we actually should keep captureLargerStackTrace on the long run.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 30, 2023

I tested it. If I patch it with err.constructor it removes the SystemError frames, but basically stackTraceLimit is broken, and instead of getting 3 frames like in my test, i get all 10 frames.

@benjamingr benjamingr removed the blocked PRs that are blocked by other issues or PRs. label Sep 30, 2023
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Oct 9, 2023

If #49990 lands, this PR should fit perfectly.

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 18, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 18, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 19, 2023

@jasnell jasnell added commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Dec 23, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 23, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/49956
✔  Done loading data for nodejs/node/pull/49956
----------------------------------- PR info ------------------------------------
Title      errors: fix stacktrace of SystemError, add more tests and benchmark (#49956)
Author     Aras Abbasi  (@Uzlopak)
Branch     Uzlopak:improve-system-error -> nodejs:main
Labels     errors
Commits    1
 - errors: fix stacktrace of SystemError
Committers 1
 - uzlopak 
PR-URL: https://github.com/nodejs/node/pull/49956
Reviewed-By: Matteo Collina 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Stephen Belanger 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/49956
Reviewed-By: Matteo Collina 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Stephen Belanger 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 29 Sep 2023 15:08:46 GMT
   ✔  Approvals: 4
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/49956#pullrequestreview-1651669625
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/49956#pullrequestreview-1651740602
   ✔  - Stephen Belanger (@Qard): https://github.com/nodejs/node/pull/49956#pullrequestreview-1669388744
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/49956#pullrequestreview-1795453904
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2023-12-19T01:35:44Z: https://ci.nodejs.org/job/node-test-pull-request/56378/
- Querying data for job/node-test-pull-request/56378/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/7309376899

@jasnell
Copy link
Member

jasnell commented Dec 23, 2023

Landed in c74c514

jasnell pushed a commit that referenced this pull request Dec 23, 2023
PR-URL: #49956
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell jasnell closed this Dec 23, 2023
RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024
PR-URL: #49956
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2024
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #49956
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-failed An error occurred while landing this pull request using GitHub Actions. errors Issues and PRs related to JavaScript errors originated in Node.js core.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants