@anonrig
@mcollina
@nodejs/performance

Is this a candidate for fast-track?

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"

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 ?

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)

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.

Actually... in captureLargerStackTrace

ErrorCaptureStackTrace(err, err.constructor);

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

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.

If #49990 lands, this PR should fit perfectly.

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

- 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

Landed in c74c514