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

inspector: fix disable async hooks on Debugger.setAsyncCallStackDepth #53473

Merged
merged 3 commits into from
Jun 18, 2024

Conversation

joyeecheung
Copy link
Member

This was previously calling the enable function by mistake. As a result, when profiling using Chrome DevTools, the async hooks won't be turned off properly after receiving Debugger.setAsyncCallStackDepth with depth 0.

This was previously calling the enable function by mistake. As a
result, when profiling using Chrome DevTools, the async hooks won't
be turned off properly after receiving Debugger.setAsyncCallStackDepth
with depth 0.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol needs-ci PRs that need a full CI run. labels Jun 16, 2024
@joyeecheung
Copy link
Member Author

Apparently there is a test in test/parallel/test-inspector-async-call-stack.js but it was not working as intended because errors thrown in the session callbacks are not handled by the process. That seems to be a different bug (or is that working as intended? I guess not? cc @nodejs/inspector ). As a result, the test was not failing even though the assertions failed.For this PR, I will just rewrite the test to use inspector/promises which allows proper error handling.

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 16, 2024
@joyeecheung
Copy link
Member Author

Added another test for checking promise hooks - it needs a utility to check the JS hooks stored in the AsyncHooks since V8 doesn't provide an API for us to check the configured JS promise hooks.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 16, 2024
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added notable-change PRs with changes that should be highlighted in changelogs. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jun 17, 2024
Copy link
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. Otherwise, the commit will be placed in the Other Notable Changes section.

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 17, 2024
@joyeecheung joyeecheung added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jun 18, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 18, 2024
@nodejs-github-bot nodejs-github-bot merged commit 0c1c33a into nodejs:main Jun 18, 2024
63 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 0c1c33a

targos pushed a commit that referenced this pull request Jun 20, 2024
This was previously calling the enable function by mistake. As a
result, when profiling using Chrome DevTools, the async hooks won't
be turned off properly after receiving Debugger.setAsyncCallStackDepth
with depth 0.

PR-URL: #53473
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
EliphazB pushed a commit to EliphazB/node that referenced this pull request Jun 20, 2024
This was previously calling the enable function by mistake. As a
result, when profiling using Chrome DevTools, the async hooks won't
be turned off properly after receiving Debugger.setAsyncCallStackDepth
with depth 0.

PR-URL: nodejs#53473
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
This was previously calling the enable function by mistake. As a
result, when profiling using Chrome DevTools, the async hooks won't
be turned off properly after receiving Debugger.setAsyncCallStackDepth
with depth 0.

PR-URL: nodejs#53473
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
targos added a commit that referenced this pull request Jun 25, 2024
Notable changes:

deps,lib,src:
  * (SEMVER-MINOR) add experimental web storage (Colin Ihrig) #52435
doc:
  * move `node --run` stability to rc (Yagiz Nizipli) #53433
  * mark WebSocket as stable (Matthew Aitken) #53352
  * mark --heap-prof and related flags stable (Joyee Cheung) #53343
  * mark --cpu-prof and related flags stable (Joyee Cheung) #53343
  * doc-only deprecate OpenSSL engine-based APIs (Richard Lau) #53329
inspector:
  * fix disable async hooks on Debugger.setAsyncCallStackDepth (Joyee Cheung) #53473
lib:
  * (SEMVER-MINOR) add diagnostics_channel events to module loading (RafaelGSS) #44340
util:
  * (SEMVER-MINOR) support `--no-` for argument with boolean type for parseArgs (Zhenwei Jin) #53107

PR-URL: #53583
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. inspector Issues and PRs related to the V8 inspector protocol needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants