fs: add stacktrace to fs/promises

[sapphi-red]

Sync functions in fs throwed an error with a stacktrace which is helpful for debugging. But functions in fs/promises throwed an error without a stacktrace. This commit adds stacktraces by calling Error.captureStacktrace and re-throwing the error.

Refs: #34817
Fixes: #50160

import fsS from 'node:fs'
fsS.readdirSync('./foo')
/*
node:fs:1515
  handleErrorFromBinding(ctx);
  ^

Error: ENOENT: no such file or directory, scandir './foo'
    at Object.readdirSync (node:fs:1515:3)
    at file:///mnt/c/users/green/Downloads/foo/index.mjs:6:5
    at ModuleJob.run (node:internal/modules/esm/module_job:218:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:328:24)
    at async loadESM (node:internal/process/esm_loader:34:7)
    at async handleMainPromise (node:internal/modules/run_main:79:12) {
  errno: -2,
  syscall: 'scandir',
  code: 'ENOENT',
  path: './foo'
}

Node.js v21.0.0-pre
*/

import fs from 'node:fs/promises'
await fs.readdir('./foo')
/*
---------- Before ----------

node:internal/process/esm_loader:40
      internalBinding('errors').triggerUncaughtException(
                                ^

[Error: ENOENT: no such file or directory, scandir './foo'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'scandir',
  path: './foo'
}

Node.js v21.0.0-pre

---------- After ----------

node:internal/fs/promises:862
  const result = await binding.readdir(
                 ^

Error: ENOENT: no such file or directory, scandir './foo'
    at async Object.readdir (node:internal/fs/promises:862:18)
    at async file:///mnt/c/users/green/Downloads/foo/index.mjs:5:1 {
  errno: -2,
  code: 'ENOENT',
  syscall: 'scandir',
  path: './foo'
}

Node.js v21.0.0-pre
*/

I have some questions.

• Was the stacktrace generation skipped on purpose? For example, in terms of performance. I remember capturing the stacktrace is slow.
• Is there a better way? I guess I can call Error.captureStackTrace on C++ land but didn't find the way.

[sapphi-red]

This now needs to be an async function otherwise binding.fstat(/* */).catch will fail.

[aduh95]

Could you try to use PromisePrototypeThen from primordials so it keeps working even if the Promise prototype have been mutated in userland?

[anonrig]

Instead of going down this path, we should throw the errors on C++ side. Similar approach is taken with Sync functions at the moment.

[sapphi-red]

Sure. I'll take another look on the c++ land.
I tried to capture the stacktrace by Exception::CreateMessage at AfterNoArgs or FSReqAfterScope::Reject but it didn't capture any stacktraces. Maybe I'm facing #34817 (comment) but I don't understand what that means yet.
I now discovered https://github.com/nodejs/node/blob/main/src/README.md so I'll read that first.

[joyeecheung]

Instead of going down this path, we should throw the errors on C++ side. Similar approach is taken with Sync functions at the moment.

We already reject directly from the C++ land (note that this is not throwing errors, this is promise rejection) and that's actually why the async stack trace is getting lost, because by the time we reject from the C++ land (note that this is async, so this is likely coming from some libuv callback execution in an event loop iteration), there is no JS frame on the stack at all. We need to reject and then await for V8 to resume back, and only until V8 resumes execution back to the JS land would there be enough information about the call site for us to re-capture a full stack trace. I don't think there is a way to resume in C++ land, so we have to do this in JS.

[joyeecheung]

Overall LGTM % some suggestions

[joyeecheung]

Can you add a test with a closure too?

[sapphi-red]

I'm not sure if I understand correctly but I added a test here.
https://github.com/nodejs/node/pull/49849/files#diff-9f28bba0b9948a16a3905ddc942f5c0a49cfd2e34214dc7ce331c7e33fb1c13fR98-R106
To make this work, I had to add await before return for zero-cost async stack traces (http://bit.ly/v8-zero-cost-async-stack-traces).
But the linter doesn't allow to do return await by the no-return-await rule.

[aduh95]

Does the 4-year-old document you link still apply on modern V8 versions? AFAIK the lint rule is there because of assumptions, we should challenge those assumptions and reassess the linter rule if necessary.

[sapphi-red]

Does the 4-year-old document you link still apply on modern V8 versions?

I'm not sure but if I remove await the test I added won't pass at least. FWIW the document is linked from https://v8.dev/docs/stack-trace-api#async-stack-traces.

AFAIK the lint rule is there because of assumptions, we should challenge those assumptions and reassess the linter rule if necessary.

The documentation says the rule is deprecated.

This rule was deprecated in ESLint v8.46.0 with no replacement. The original intent of this rule no longer applies due to the fact JavaScript now handles native Promises differently. It can now be slower to remove await rather than keeping it. More technical information can be found in this V8 blog entry.

[aduh95]

If you open a PR to remove that rule, it will probably get accepted.

[joyeecheung]

Also to answer questions from the OP:

Was the stacktrace generation skipped on purpose? For example, in terms of performance. I remember capturing the stacktrace is slow.

I don't think skipping async stack generation is done on purpose, it's likely an oversight. This is also done on the error path so the success path is unaffected. In this case I think having an actually meaningful stack trace with a bit of overhead is much more important than generating a useless stack trace quickly.

Is there a better way? I guess I can call Error.captureStackTrace on C++ land but didn't find the way.

For now I think resuming back to JS land and capture stack trace there is probably the best we can do. Note that it's not a matter of where you capture the stack trace, it's when you capture the stack trace. When the system call encounter an error, libuv invokes our callback in an event loop iteration, and we create and error and then reject. But at this point in time, there is no JS stack frame on the stack (this is why we have no JS frames for async stack trace but we have them for the sync ones - by the time we create JS errors in a synchronous method, no matter in JS or C++, the call site information is still there on the stack, V8 can do some lightweight capture for that and defer the expensive stringification until error.stack is actually accessed. The same can't be said for async errors coming from a libuv callback). Even if you capture the stack trace before you reject, you still end up with the same useless stack trace because you are still in a C++ callback with no JS frames on the stack for V8 to capture. There are two possible approach we can do to solve this:

1. We always keep track of the async execution context until the promise can be resolved, and generate the stack trace on our own in C++ without going back to JS.
2. We do what this PR does - let V8 keep the async execution context under the hood for the await implementation, and piggy-back on that to re-capture the stack trace when the execution is resumed in JS land.

I think 2 is much simpler than 1, also 1 can cost quite a bit of additional memory and it'll go to waste if the system call does not fail (which is the more likely case), whereas V8 already maintains the async execution context more performantly under the hood for async-await, so we might as well just use that.

[github-actions]

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @joyeecheung.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment.

Invalid

[Uzlopak]

is there a perf difference when doing

Suggested change

	ErrorCaptureStackTrace(error, handleErrorFromBinding);
	Error.stackTraceLimit && ErrorCaptureStackTrace(error, handleErrorFromBinding);

So only call ErrorCaptureStackTrace if Error.strackTraceLimit is not false (= 0)

[sapphi-red]

the code of benchmark

'use strict';

const common = require('../common');
const fs = require('fs/promises');

const bench = common.createBenchmark(main, {
  n: [1e5],
  stacktraceLimit: [0, 10]
});


async function main({ n, stacktraceLimit }) {
  const fn = fs.access;
  Error.stackTraceLimit = stacktraceLimit;

  bench.start();
  for (let i = 0; i < n; i++) {
    try {
      await fn('no existance');
    } catch {}
  }
  bench.end(n);
}

The result of the benchmark above on my machine is

// without Error.stackTraceLimit
fs/foo.js stacktraceLimit=0 n=100000: 25,569.003187542396
fs/foo.js stacktraceLimit=10 n=100000: 24,136.711560423188

// with Error.stackTraceLimit
fs/foo.js stacktraceLimit=0 n=100000: 25,789.21066371575
fs/foo.js stacktraceLimit=10 n=100000: 23,625.22690199484

[Uzlopak]

Thank you

[sapphi-red]

Also to answer questions from the OP:

Thank you for the answers!

[sapphi-red]

I rebased on main branch to include #50118 so that the lint passes.

[github-actions]

Failed to start CI

- Validating Jenkins credentials
✘  Jenkins credentials invalid

https://github.com/nodejs/node/actions/runs/6497069197

[sapphi-red]

@UlisesGascon Sure! Opened it: #51127

[mika-fischer]

For now I think resuming back to JS land and capture stack trace there is probably the best we can do. Note that it's not a matter of where you capture the stack trace, it's when you capture the stack trace. When the system call encounter an error, libuv invokes our callback in an event loop iteration, and we create and error and then reject. But at this point in time, there is no JS stack frame on the stack (this is why we have no JS frames for async stack trace but we have them for the sync ones - by the time we create JS errors in a synchronous method, no matter in JS or C++, the call site information is still there on the stack, V8 can do some lightweight capture for that and defer the expensive stringification until error.stack is actually accessed. The same can't be said for async errors coming from a libuv callback). Even if you capture the stack trace before you reject, you still end up with the same useless stack trace because you are still in a C++ callback with no JS frames on the stack for V8 to capture. There are two possible approach we can do to solve this:

1. We always keep track of the async execution context until the promise can be resolved, and generate the stack trace on our own in C++ without going back to JS.
2. We do what this PR does - let V8 keep the async execution context under the hood for the await implementation, and piggy-back on that to re-capture the stack trace when the execution is resumed in JS land.

@joyeecheung Sorry to hijack this but would you mind elaborating a bit on option 1? We have a custom wrapper for our C++ library using napi and we have this same issue of empty stack traces when rejecting a promise from C++. I wanted to evaluate whether option 1 would be a better match for us than option 2. But I can't get it to work.

What I tried is:

• On async initiation:
  • napi_async_init
  • napi_create_promise
  • store the napi_async_context in an std::map<napi_deferred, napi_async_context>
• On rejection:
  • retrieve the napi_async_context from the map
  • napi_open_callback_scope
  • the napi equivalent of Error.captureStackTrace(rejection) (using napi_call_function or napi_make_callback)
  • napi_reject_deferred
  • napi_close_callback_scope
  • napi_async_destroy

However the rejection still has no stack trace. Is this different to what you mean by option 1 above. Thanks for the help!

[joyeecheung]

However the rejection still has no stack trace. Is this different to what you mean by option 1 above.

By "async context" I did not mean napi_async_context, but a conceptual "some structure we need to maintain ourselves to keep track every time we do a C++ -> JS callback, and it should save the user caller function information in there, somehow"

[mika-fischer]

By "async context" I did not mean napi_async_context, but a conceptual "some structure we need to maintain ourselves to keep track every time we do a C++ -> JS callback, and it should save the user caller function information in there, somehow"

I see, so the infrastructure to implement option 1 does not readily exist, much less so in napi. Fair enough, then we need to see how to implement option 2.

Thanks for the input!