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

Long stack traces in core #31080

Closed
addaleax opened this issue Dec 24, 2019 · 15 comments
Closed

Long stack traces in core #31080

addaleax opened this issue Dec 24, 2019 · 15 comments
Labels
async_hooks Issues and PRs related to the async hooks subsystem. feature request Issues that request new features to be added to Node.js.

Comments

@addaleax
Copy link
Member

With all the work currently being done around async_hooks and stack trace enhancement through e.g. source maps, I’m wondering whether we should bring official long stack trace support into core.

I’d personally love to see it – I’m certainly biased, but for debugging flaky tests alone it would make things a lot easier to not to have to pull in an external utility for that (or to have to write an ad-hoc one).

Alternatives would be to keep these utilities in userland and/or to just vendor one in but keep developing it outside of core. In particular the latter also seems good to me.

@nodejs/async_hooks @bcoe @AndreasMadsen

@addaleax addaleax added async_hooks Issues and PRs related to the async hooks subsystem. feature request Issues that request new features to be added to Node.js. labels Dec 24, 2019
@benjamingr
Copy link
Member

I'm not sure what that would entail.

Also - do you mean for things other than async functions?

I was under the impression that we already do have long stack traces in core (for async functions) with --async-stack-traces

@benjamingr
Copy link
Member

I am obviously a big +1 on anything that makes debugging async code easier :)

@addaleax
Copy link
Member Author

@benjamingr I’m talking about any async resources, not just async functions here :) So including nextTicks, timers, etc.

@cjihrig
Copy link
Contributor

cjihrig commented Dec 24, 2019

I'm +1 on the idea in general.

Alternatives would be to keep these utilities in userland and/or to just vendor one in but keep developing it outside of core.

I'd be OK with that if the vendored module behaved like any other core module. For example, when we added rimraf support, the userland module didn't really have the same developer experience, mostly around things like option names and error handling.

@gireeshpunathil
Copy link
Member

sorry for asking a basic question: what is a long stack trace? a simulated chain of stack traces that belong to one logical call unit but were never available together in the thread stack due to the presences of async calls? or is it something else?

@addaleax
Copy link
Member Author

@gireeshpunathil Yes, exactly.

@joyeecheung
Copy link
Member

joyeecheung commented Dec 24, 2019

I guess the confusion comes from that the concept is usually referred to as "async stack trace"? (e.g. the asyncStackTrace implemented by our inspector agent when Debugger.setAsyncCallStackDepth is sent, which is implemented using async hooks). So essentially this means we are going to always do this instead of reserving it for handling Debugger.setAsyncCallStackDepth in the inspector?

@addaleax
Copy link
Member Author

@joyeecheung Not always – I’m thinking that this would be enabled through e.g. a command line flag.

@joyeecheung
Copy link
Member

@addaleax I see. I would be inclined to maintaining this in core instead of vendoring in one in considering we already have an implementation in Node.js core for the inspector. We could essentially just refactor that code so that it reports to something that can be fetched by e.g. our stack trace preparation callback. (this again brings up the issue about making it play along with user-land Error.prepareStackTrace though)

@addaleax
Copy link
Member Author

(this again brings up the issue about making it play along with user-land Error.prepareStackTrace though)

I, personally, would be consider a solution along the lines of #30984 to be perfect (e.g. adding callFrame.asyncDepth()), but I’m afraid there’s going to be opposition against that like there is for the other PR.

@bcoe
Copy link
Contributor

bcoe commented Dec 24, 2019

@addaleax @joyeecheung just seeing this. I liked @joyeecheung's suggestion of potentially adding an additional parameter to prepareStackTrace, which could be used to lookup additional interesting information about the stack frame. I can see why folks were squeamish about simply packing this information on to the stack frame objects that originate in (I believe) V8?

Perhaps we could hammer out a little RFC document for this proposal?

@joyeecheung
Copy link
Member

joyeecheung commented Dec 24, 2019

@bcoe To clarify I was just suggesting fetching the information in our internal stack trace preparation callback, which calls the user-land Error.prepareStackTrace. To also expose something to the user-land hook could be a nice-to-have, but it seems complicated so I would not consider that a blocker (first of all, be aware that we currently always respect the user-landError.prepareStackTrace from the main context so it's easy to mess up across contexts if we are not careful).

@reklatsmasters
Copy link
Contributor

It also should have an option to return stack trace only for userland code.

@mcollina
Copy link
Member

I think this was partially solved in latests V8 with long stack traces with async functions, I'm going to close this.

@darky
Copy link
Contributor

darky commented Jan 31, 2022

What about process.nextTick, timeouts and so on?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

No branches or pull requests

9 participants