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 — fs: add stacktrace to fs/promises by sapphi-red · Pull Request #49849 · nodejs/node

Conversation

@sapphi-red

@sapphi-red sapphi-red commented Sep 25, 2023 •
edited by aduh95
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
benjamingr, chaos-zhu, mizdra, progfay, and matmen reacted with thumbs up emoji meyfa, wa-Nadoo, and isker reacted with heart emoji
All reactions
  • 👍 5 reactions
  • ❤️ 3 reactions
@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to file-system APIs and the fs module. needs-ci PRs that need a full CI run. labels Sep 25, 2023
sapphi-red
sapphi-red commented Sep 25, 2023
View reviewed changes
Comment thread test/parallel/test-fs-promises-readfile.js Outdated
Comment on lines 74 to 76

@sapphi-red sapphi-red Sep 25, 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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
aduh95
aduh95 reviewed Sep 25, 2023
View reviewed changes

@aduh95 aduh95 left a comment

Copy link
Copy Markdown
Contributor

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sapphi-red reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
anonrig
anonrig previously requested changes Sep 25, 2023
View reviewed changes

@anonrig anonrig 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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@sapphi-red

sapphi-red commented Sep 26, 2023

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

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

joyeecheung commented Oct 4, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

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.
sapphi-red and benjamingr reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
joyeecheung
joyeecheung approved these changes Oct 4, 2023
View reviewed changes

@joyeecheung joyeecheung 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.

Overall LGTM % some suggestions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-fs-promises.js Outdated

@joyeecheung joyeecheung Oct 4, 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.

Can you add a test with a closure too?

Sorry, something went wrong.

Uh oh!

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

@sapphi-red sapphi-red Oct 6, 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.

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.

Sorry, something went wrong.

Uh oh!

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

@aduh95 aduh95 Oct 6, 2023

Copy link
Copy Markdown
Contributor

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 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.

Sorry, something went wrong.

Uh oh!

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

@sapphi-red sapphi-red Oct 6, 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 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.

Sorry, something went wrong.

Uh oh!

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

@aduh95 aduh95 Oct 6, 2023

Copy link
Copy Markdown
Contributor

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sapphi-red reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread lib/internal/fs/promises.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/fs/promises.js Resolved

Uh oh!

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

joyeecheung commented Oct 4, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

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.
sapphi-red reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung joyeecheung added the notable-change PRs with changes that should be highlighted in changelogs. label Oct 4, 2023
@github-actions

github-actions Bot commented Oct 4, 2023

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig
anonrig dismissed their stale review October 4, 2023 12:29

Invalid

Uzlopak
Uzlopak reviewed Oct 4, 2023
View reviewed changes
Comment thread lib/internal/fs/promises.js Outdated

@Uzlopak Uzlopak Oct 4, 2023

Copy link
Copy Markdown
Contributor

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.

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)

Sorry, something went wrong.

Uh oh!

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

@sapphi-red sapphi-red Oct 6, 2023 •
edited
Loading

Uh oh!

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

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 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
himself65 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@Uzlopak Uzlopak Oct 6, 2023

Copy link
Copy Markdown
Contributor

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.

Thank you

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@sapphi-red

sapphi-red commented Oct 6, 2023

Copy link
Copy Markdown
Contributor Author

Also to answer questions from the OP:

Thank you for the answers!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sapphi-red sapphi-red mentioned this pull request Oct 10, 2023
tools: remove no-return-await lint rule #50118
Merged
sapphi-red and others added 5 commits October 12, 2023 20:32
@sapphi-red
fs: add stacktrace to fs/promises
b303274 
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: nodejs#34817
@sapphi-red
fixup! make it work with closures as well
017ac3c
@sapphi-red
fixup! use PromisePrototypeThen instead of .catch
92016f0
@sapphi-red @joyeecheung
fixup! add comment to handleErrorFromBinding
984d81c 
Co-authored-by: Joyee Cheung <[email protected]>
@sapphi-red
fixup! fix lint issues
b7d6550
@sapphi-red
sapphi-red force-pushed the stacktrace-for-async-fs branch from e531277 to b7d6550 Compare October 12, 2023 11:32
@sapphi-red sapphi-red mentioned this pull request Oct 12, 2023
more useable fs/promises backtraces on error #50160
Closed
@sapphi-red

sapphi-red commented Oct 12, 2023

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung joyeecheung 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 12, 2023
@github-actions github-actions Bot added request-ci-failed Starting CI with the request-ci label failed and requires manual intervention. and removed request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Oct 12, 2023
@github-actions

github-actions Bot commented Oct 12, 2023

Copy link
Copy Markdown
Contributor
Failed to start CI
- Validating Jenkins credentials
✘  Jenkins credentials invalid
https://github.com/nodejs/node/actions/runs/6497069197
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95 aduh95 added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. and removed request-ci-failed Starting CI with the request-ci label failed and requires manual intervention. labels Oct 12, 2023
benjamingr
benjamingr approved these changes Oct 13, 2023
View reviewed changes
@sapphi-red sapphi-red mentioned this pull request Dec 12, 2023
[v20.x backport] fs: add stacktrace to fs/promises #51127
Closed
@sapphi-red

sapphi-red commented Dec 12, 2023

Copy link
Copy Markdown
Contributor Author

@UlisesGascon Sure! Opened it: #51127
UlisesGascon reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sapphi-red added a commit to sapphi-red/node that referenced this pull request Mar 26, 2024
@sapphi-red
fs: add stacktrace to fs/promises
518d819 
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: nodejs#34817
PR-URL: nodejs#49849
Backport-PR-URL: nodejs#51127
Fixes: nodejs#50160
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Apr 29, 2024
@sapphi-red @marco-ippolito
fs: add stacktrace to fs/promises
c530520 
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
PR-URL: #49849
Backport-PR-URL: #51127
Fixes: #50160
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
@marco-ippolito marco-ippolito mentioned this pull request May 2, 2024
v20.13.0 proposal #52793
Merged
marco-ippolito added a commit that referenced this pull request May 2, 2024
@marco-ippolito
2024-05-07, Version 20.13.0 'Iron' (LTS)
2e68800 
Notable changes:

benchmark:
  * add AbortSignal.abort benchmarks (Raz Luvaton) #52408
buffer:
  * improve `base64` and `base64url` performance (Yagiz Nizipli) #52428
crypto:
  * deprecate implicitly shortened GCM tags (Tobias Nießen) #52345
deps:
  * (SEMVER-MINOR) update simdutf to 5.0.0 (Daniel Lemire) #52138
  * (SEMVER-MINOR) update undici to 6.3.0 (Node.js GitHub Bot) #51462
  * (SEMVER-MINOR) update undici to 6.2.1 (Node.js GitHub Bot) #51278
dns:
  * (SEMVER-MINOR) add order option and support ipv6first (Paolo Insogna) #52492
doc:
  * update release gpg keyserver (marco-ippolito) #52257
  * add release key for marco-ippolito (marco-ippolito) #52257
  * add UlisesGascon as a collaborator (Ulises Gascón) #51991
  * (SEMVER-MINOR) deprecate fs.Stats public constructor (Marco Ippolito) #51879
events,doc:
  * mark CustomEvent as stable (Daeyeon Jeong) #52618
fs:
  * add stacktrace to fs/promises (翠 / green) #49849
lib, url:
  * (SEMVER-MINOR) add a `windows` option to path parsing (Aviv Keller) #52509
net:
  * (SEMVER-MINOR) add CLI option for autoSelectFamilyAttemptTimeout (Paolo Insogna) #52474
report:
  * (SEMVER-MINOR) add `--report-exclude-network` option (Ethan Arrowood) #51645
src:
  * (SEMVER-MINOR) add `string_view` overload to snapshot FromBlob (Anna Henningsen) #52595
  * (SEMVER-MINOR) add C++ ProcessEmitWarningSync() (Joyee Cheung) #51977
  * (SEMVER-MINOR) add uv_get_available_memory to report and process (theanarkh) #52023
  * (SEMVER-MINOR) preload function for Environment (Cheng Zhao) #51539
stream:
  * (SEMVER-MINOR) support typed arrays (IlyasShabi) #51866
test_runner:
  * (SEMVER-MINOR) add suite() (Colin Ihrig) #52127
  * (SEMVER-MINOR) support forced exit (Colin Ihrig) #52038
  * (SEMVER-MINOR) add `test:complete` event to reflect execution order (Moshe Atlow) #51909
util:
  * (SEMVER-MINOR) support array of formats in util.styleText (Marco Ippolito) #52040
v8:
  * (SEMVER-MINOR) implement v8.queryObjects() for memory leak regression testing (Joyee Cheung) #51927
watch:
  * mark as stable (Moshe Atlow) #52074

PR-URL: #52793
marco-ippolito added a commit that referenced this pull request May 3, 2024
@marco-ippolito
2024-05-07, Version 20.13.0 'Iron' (LTS)
1766b1d 
Notable changes:

benchmark:
  * add AbortSignal.abort benchmarks (Raz Luvaton) #52408
buffer:
  * improve `base64` and `base64url` performance (Yagiz Nizipli) #52428
crypto:
  * deprecate implicitly shortened GCM tags (Tobias Nießen) #52345
deps:
  * (SEMVER-MINOR) update simdutf to 5.0.0 (Daniel Lemire) #52138
  * (SEMVER-MINOR) update undici to 6.3.0 (Node.js GitHub Bot) #51462
  * (SEMVER-MINOR) update undici to 6.2.1 (Node.js GitHub Bot) #51278
dns:
  * (SEMVER-MINOR) add order option and support ipv6first (Paolo Insogna) #52492
doc:
  * update release gpg keyserver (marco-ippolito) #52257
  * add release key for marco-ippolito (marco-ippolito) #52257
  * add UlisesGascon as a collaborator (Ulises Gascón) #51991
  * (SEMVER-MINOR) deprecate fs.Stats public constructor (Marco Ippolito) #51879
events,doc:
  * mark CustomEvent as stable (Daeyeon Jeong) #52618
fs:
  * add stacktrace to fs/promises (翠 / green) #49849
lib, url:
  * (SEMVER-MINOR) add a `windows` option to path parsing (Aviv Keller) #52509
net:
  * (SEMVER-MINOR) add CLI option for autoSelectFamilyAttemptTimeout (Paolo Insogna) #52474
report:
  * (SEMVER-MINOR) add `--report-exclude-network` option (Ethan Arrowood) #51645
src:
  * (SEMVER-MINOR) add `string_view` overload to snapshot FromBlob (Anna Henningsen) #52595
  * (SEMVER-MINOR) add C++ ProcessEmitWarningSync() (Joyee Cheung) #51977
  * (SEMVER-MINOR) add uv_get_available_memory to report and process (theanarkh) #52023
  * (SEMVER-MINOR) preload function for Environment (Cheng Zhao) #51539
stream:
  * (SEMVER-MINOR) support typed arrays (IlyasShabi) #51866
test_runner:
  * (SEMVER-MINOR) add suite() (Colin Ihrig) #52127
  * (SEMVER-MINOR) add `test:complete` event to reflect execution order (Moshe Atlow) #51909
util:
  * (SEMVER-MINOR) support array of formats in util.styleText (Marco Ippolito) #52040
v8:
  * (SEMVER-MINOR) implement v8.queryObjects() for memory leak regression testing (Joyee Cheung) #51927
watch:
  * mark as stable (Moshe Atlow) #52074

PR-URL: #52793
marco-ippolito added a commit that referenced this pull request May 7, 2024
@marco-ippolito
2024-05-07, Version 20.13.0 'Iron' (LTS)
0d0efa4 
Notable changes:

benchmark:
  * add AbortSignal.abort benchmarks (Raz Luvaton) #52408
buffer:
  * improve `base64` and `base64url` performance (Yagiz Nizipli) #52428
crypto:
  * deprecate implicitly shortened GCM tags (Tobias Nießen) #52345
deps:
  * (SEMVER-MINOR) update simdutf to 5.0.0 (Daniel Lemire) #52138
  * (SEMVER-MINOR) update undici to 6.3.0 (Node.js GitHub Bot) #51462
  * (SEMVER-MINOR) update undici to 6.2.1 (Node.js GitHub Bot) #51278
dns:
  * (SEMVER-MINOR) add order option and support ipv6first (Paolo Insogna) #52492
doc:
  * update release gpg keyserver (marco-ippolito) #52257
  * add release key for marco-ippolito (marco-ippolito) #52257
  * add UlisesGascon as a collaborator (Ulises Gascón) #51991
  * (SEMVER-MINOR) deprecate fs.Stats public constructor (Marco Ippolito) #51879
events,doc:
  * mark CustomEvent as stable (Daeyeon Jeong) #52618
fs:
  * add stacktrace to fs/promises (翠 / green) #49849
lib, url:
  * (SEMVER-MINOR) add a `windows` option to path parsing (Aviv Keller) #52509
net:
  * (SEMVER-MINOR) add CLI option for autoSelectFamilyAttemptTimeout (Paolo Insogna) #52474
report:
  * (SEMVER-MINOR) add `--report-exclude-network` option (Ethan Arrowood) #51645
src:
  * (SEMVER-MINOR) add `string_view` overload to snapshot FromBlob (Anna Henningsen) #52595
  * (SEMVER-MINOR) add C++ ProcessEmitWarningSync() (Joyee Cheung) #51977
  * (SEMVER-MINOR) add uv_get_available_memory to report and process (theanarkh) #52023
  * (SEMVER-MINOR) preload function for Environment (Cheng Zhao) #51539
stream:
  * (SEMVER-MINOR) support typed arrays (IlyasShabi) #51866
test_runner:
  * (SEMVER-MINOR) add suite() (Colin Ihrig) #52127
  * (SEMVER-MINOR) add `test:complete` event to reflect execution order (Moshe Atlow) #51909
util:
  * (SEMVER-MINOR) support array of formats in util.styleText (Marco Ippolito) #52040
v8:
  * (SEMVER-MINOR) implement v8.queryObjects() for memory leak regression testing (Joyee Cheung) #51927
watch:
  * mark as stable (Moshe Atlow) #52074

PR-URL: #52793
marco-ippolito added a commit that referenced this pull request May 7, 2024
@marco-ippolito
2024-05-07, Version 20.13.0 'Iron' (LTS)
9a1df15 
Notable changes:

benchmark:
  * add AbortSignal.abort benchmarks (Raz Luvaton) #52408
buffer:
  * improve `base64` and `base64url` performance (Yagiz Nizipli) #52428
crypto:
  * deprecate implicitly shortened GCM tags (Tobias Nießen) #52345
deps:
  * (SEMVER-MINOR) update simdutf to 5.0.0 (Daniel Lemire) #52138
  * (SEMVER-MINOR) update undici to 6.3.0 (Node.js GitHub Bot) #51462
  * (SEMVER-MINOR) update undici to 6.2.1 (Node.js GitHub Bot) #51278
dns:
  * (SEMVER-MINOR) add order option and support ipv6first (Paolo Insogna) #52492
doc:
  * update release gpg keyserver (marco-ippolito) #52257
  * add release key for marco-ippolito (marco-ippolito) #52257
  * add UlisesGascon as a collaborator (Ulises Gascón) #51991
  * (SEMVER-MINOR) deprecate fs.Stats public constructor (Marco Ippolito) #51879
events,doc:
  * mark CustomEvent as stable (Daeyeon Jeong) #52618
fs:
  * add stacktrace to fs/promises (翠 / green) #49849
lib, url:
  * (SEMVER-MINOR) add a `windows` option to path parsing (Aviv Keller) #52509
net:
  * (SEMVER-MINOR) add CLI option for autoSelectFamilyAttemptTimeout (Paolo Insogna) #52474
report:
  * (SEMVER-MINOR) add `--report-exclude-network` option (Ethan Arrowood) #51645
src:
  * (SEMVER-MINOR) add `string_view` overload to snapshot FromBlob (Anna Henningsen) #52595
  * (SEMVER-MINOR) add C++ ProcessEmitWarningSync() (Joyee Cheung) #51977
  * (SEMVER-MINOR) add uv_get_available_memory to report and process (theanarkh) #52023
  * (SEMVER-MINOR) preload function for Environment (Cheng Zhao) #51539
stream:
  * (SEMVER-MINOR) support typed arrays (IlyasShabi) #51866
test_runner:
  * (SEMVER-MINOR) add suite() (Colin Ihrig) #52127
  * (SEMVER-MINOR) add `test:complete` event to reflect execution order (Moshe Atlow) #51909
util:
  * (SEMVER-MINOR) support array of formats in util.styleText (Marco Ippolito) #52040
v8:
  * (SEMVER-MINOR) implement v8.queryObjects() for memory leak regression testing (Joyee Cheung) #51927
watch:
  * mark as stable (Moshe Atlow) #52074

PR-URL: #52793
codebytere added a commit to electron/electron that referenced this pull request May 10, 2024
@codebytere
fs: add stacktrace to fs/promises
7124bf4 
nodejs/node#49849
jkleinsc pushed a commit to electron/electron that referenced this pull request May 13, 2024
@electron-roller @codebytere @patchup
chore: bump node to v20.13.1 (main) (#42088)
653d0f0 
* chore: bump node in DEPS to v20.13.0

* crypto: enable NODE_EXTRA_CA_CERTS with BoringSSL

nodejs/node#52217

* test: skip test for dynamically linked OpenSSL

nodejs/node#52542

* lib, url: add a `windows` option to path parsing

nodejs/node#52509

* src: use dedicated routine to compile function for builtin CJS loader

nodejs/node#52016

* test: mark test as flaky

nodejs/node#52671

* build,tools: add test-ubsan ci

nodejs/node#46297

* src: preload function for Environment

nodejs/node#51539

* chore: fixup patch indices

* deps: update c-ares to 1.28.1

nodejs/node#52285

* chore: handle updated filenames

- nodejs/node#51999
- nodejs/node#51927

* chore: bump node in DEPS to v20.13.1

* events: extract addAbortListener for safe internal use

nodejs/node#52081

* module: print location of unsettled top-level await in entry points

nodejs/node#51999

* fs: add stacktrace to fs/promises

nodejs/node#49849

* chore: update patches

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <[email protected]>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
codebytere added a commit to electron/electron that referenced this pull request May 31, 2024
@codebytere
fs: add stacktrace to fs/promises
867466c 
nodejs/node#49849
codebytere added a commit to electron/electron that referenced this pull request May 31, 2024
@codebytere
fs: add stacktrace to fs/promises
ed9bdb8 
nodejs/node#49849
codebytere added a commit to electron/electron that referenced this pull request May 31, 2024
@codebytere
fs: add stacktrace to fs/promises
b797aec 
nodejs/node#49849
codebytere added a commit to electron/electron that referenced this pull request May 31, 2024
@electron-roller @codebytere
chore: bump node to v20.14.0 (31-x-y) (#42295)
9a87938 
* chore: bump node in DEPS to v20.13.1

* chore: bump node in DEPS to v20.14.0

* crypto: enable NODE_EXTRA_CA_CERTS with BoringSSL

nodejs/node#52217

* test: skip test for dynamically linked OpenSSL

nodejs/node#52542

* lib, url: add a `windows` option to path parsing

nodejs/node#52509

* src: use dedicated routine to compile function for builtin CJS loader

nodejs/node#52016

* test: mark test as flaky

nodejs/node#52671

* build,tools: add test-ubsan ci

nodejs/node#46297

* src: preload function for Environment

nodejs/node#51539

* chore: fixup patch indices

* deps: update c-ares to 1.28.1

nodejs/node#52285

* chore: handle updated filenames

* events: extract addAbortListener for safe internal use

nodejs/node#52081

* module: print location of unsettled top-level await in entry points

nodejs/node#51999

* fs: add stacktrace to fs/promises

nodejs/node#49849

* chore: fixup patch indices

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <[email protected]>
codebytere added a commit to electron/electron that referenced this pull request Jun 1, 2024
@electron-roller @zcbenz
@codebytere @patchup
chore: bump node to v20.14.0 (30-x-y) (#42294)
3be3381 
* chore: bump node in DEPS to v20.13.1

* chore: bump node in DEPS to v20.14.0

* chore: update build_add_gn_build_files.patch

* chore: update patches

* chore: update patches

* build: encode non-ASCII Latin1 characters as one byte in JS2C

nodejs/node#51605

* crypto: use EVP_MD_fetch and cache EVP_MD for hashes

nodejs/node#51034

* chore: update filenames.json

* chore: update patches

* src: support configurable snapshot

nodejs/node#50453

* test: remove test-domain-error-types flaky designation

nodejs/node#51717

* src: avoid draining platform tasks at FreeEnvironment

nodejs/node#51290

* chore: fix accidentally deleted v8 dep

* lib: define FormData and fetch etc. in the built-in snapshot

nodejs/node#51598

* chore: remove stray log

* crypto: enable NODE_EXTRA_CA_CERTS with BoringSSL

nodejs/node#52217

* test: skip test for dynamically linked OpenSSL

nodejs/node#52542

* lib, url: add a `windows` option to path parsing

nodejs/node#52509

* src: use dedicated routine to compile function for builtin CJS loader

nodejs/node#52016

* test: mark test as flaky

nodejs/node#52671

* build,tools: add test-ubsan ci

nodejs/node#46297

* src: preload function for Environment

nodejs/node#51539

* deps: update c-ares to 1.28.1

nodejs/node#52285

* chore: fixup

* events: extract addAbortListener for safe internal use

nodejs/node#52081

* module: print location of unsettled top-level await in entry points

nodejs/node#51999

* fs: add stacktrace to fs/promises

nodejs/node#49849

* chore: fixup indices

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Cheng <[email protected]>
Co-authored-by: Shelley Vohr <[email protected]>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
@marco-ippolito @soophoo
2024-05-07, Version 20.13.0 'Iron' (LTS)
66ad117 
Notable changes:

benchmark:
  * add AbortSignal.abort benchmarks (Raz Luvaton) nodejs#52408
buffer:
  * improve `base64` and `base64url` performance (Yagiz Nizipli) nodejs#52428
crypto:
  * deprecate implicitly shortened GCM tags (Tobias Nießen) nodejs#52345
deps:
  * (SEMVER-MINOR) update simdutf to 5.0.0 (Daniel Lemire) nodejs#52138
  * (SEMVER-MINOR) update undici to 6.3.0 (Node.js GitHub Bot) nodejs#51462
  * (SEMVER-MINOR) update undici to 6.2.1 (Node.js GitHub Bot) nodejs#51278
dns:
  * (SEMVER-MINOR) add order option and support ipv6first (Paolo Insogna) nodejs#52492
doc:
  * update release gpg keyserver (marco-ippolito) nodejs#52257
  * add release key for marco-ippolito (marco-ippolito) nodejs#52257
  * add UlisesGascon as a collaborator (Ulises Gascón) nodejs#51991
  * (SEMVER-MINOR) deprecate fs.Stats public constructor (Marco Ippolito) nodejs#51879
events,doc:
  * mark CustomEvent as stable (Daeyeon Jeong) nodejs#52618
fs:
  * add stacktrace to fs/promises (翠 / green) nodejs#49849
lib, url:
  * (SEMVER-MINOR) add a `windows` option to path parsing (Aviv Keller) nodejs#52509
net:
  * (SEMVER-MINOR) add CLI option for autoSelectFamilyAttemptTimeout (Paolo Insogna) nodejs#52474
report:
  * (SEMVER-MINOR) add `--report-exclude-network` option (Ethan Arrowood) nodejs#51645
src:
  * (SEMVER-MINOR) add `string_view` overload to snapshot FromBlob (Anna Henningsen) nodejs#52595
  * (SEMVER-MINOR) add C++ ProcessEmitWarningSync() (Joyee Cheung) nodejs#51977
  * (SEMVER-MINOR) add uv_get_available_memory to report and process (theanarkh) nodejs#52023
  * (SEMVER-MINOR) preload function for Environment (Cheng Zhao) nodejs#51539
stream:
  * (SEMVER-MINOR) support typed arrays (IlyasShabi) nodejs#51866
test_runner:
  * (SEMVER-MINOR) add suite() (Colin Ihrig) nodejs#52127
  * (SEMVER-MINOR) add `test:complete` event to reflect execution order (Moshe Atlow) nodejs#51909
util:
  * (SEMVER-MINOR) support array of formats in util.styleText (Marco Ippolito) nodejs#52040
v8:
  * (SEMVER-MINOR) implement v8.queryObjects() for memory leak regression testing (Joyee Cheung) nodejs#51927
watch:
  * mark as stable (Moshe Atlow) nodejs#52074

PR-URL: nodejs#52793
@mika-fischer

mika-fischer commented Sep 26, 2024

Copy link
Copy Markdown
Contributor

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!
All reactions

Sorry, something went wrong.

Uh oh!

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

joyeecheung commented Sep 26, 2024

Copy link
Copy Markdown
Member

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"
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mika-fischer

mika-fischer commented Sep 26, 2024

Copy link
Copy Markdown
Contributor

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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@coderabbitai coderabbitai Bot mentioned this pull request Aug 6, 2026
feat(stagewise): support isolated dev instances stagewise-io/stagewise#1524
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes
@aduh95 aduh95 aduh95 left review comments
@benjamingr benjamingr benjamingr approved these changes
@gengjiawen gengjiawen gengjiawen approved these changes
@MoLow MoLow MoLow approved these changes
@himself65 himself65 himself65 approved these changes
@anonrig anonrig anonrig left review comments
+1 more reviewer
@Uzlopak Uzlopak Uzlopak left review comments
Reviewers whose approvals may not affect merge requirements

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. commit-queue-squash PRs the Commit Queue should land as one squashed commit. fs Issues and PRs related to file-system APIs and the fs module. needs-benchmark-ci PRs that need a benchmark CI run. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

more useable fs/promises backtraces on error

Uh oh!

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

12 participants

@sapphi-red @joyeecheung @nodejs-github-bot @UlisesGascon @mika-fischer @benjamingr @anonrig @gengjiawen @Uzlopak @MoLow @himself65 @aduh95
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.